Refactor backend to be API first, with dedicated frontend#351
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adds a Vue 3 frontend (built/embedded), implements OIDC + JWT auth (UI cookie and CLI token flows), reworks HTTP routing to an API-first model with SPA support and middleware, converts the bind CLI to a UI-driven callback flow with binder tooling, updates SDK CRD types (rename + new request types), adds multi-stage multi-arch Docker builds and Makefile targets, and adapts e2e tests and docs to the new flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser (UI)
participant CLI as kubectl-bind (CLI)
participant Server as kube-bind (Backend)
participant OIDC as OIDC Provider
CLI->>Server: POST /api/authorize {session_id, redirect_url, client_type=cli}
Browser->>Server: POST /api/authorize {session_id, redirect_url, client_type=ui}
Server->>OIDC: Redirect to OIDC authorize (state encodes session)
OIDC->>Server: GET /api/callback?code&state
Server->>Server: Exchange code -> tokens, create session / JWT
alt CLI flow
Server->>CLI: Redirect to callback_url?access_token=JWT
else UI flow
Server->>Browser: Set secure session cookie and redirect to /resources
end
Browser->>Server: GET /api/templates (cookie auth)
CLI->>Server: GET /api/templates (bearer JWT)
Server->>Server: AuthMiddleware validates cookie or JWT
Server->>Browser/CLI: Respond JSON (templates / bind result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing focused review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
5fe3d18 to
28a13a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (21)
sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
75-82: Documentation improvement approved; optional marker is redundant.The capitalization of "Namespaces" in the documentation improves consistency with the field name.
The addition of
+kubebuilder:validation:Optionalon line 81 is redundant since the field is already marked with+optionalon line 80 and fields are optional by default in kubebuilder unless explicitly marked as required. Note that thePermissionClaimsfield (lines 72-73) uses only the+optionalmarker without the explicit validation annotation, creating a minor inconsistency within this file.backend/static/embed.go (1)
26-36: Add period to comment on line 27.The code changes are correct. The
gofmtcommand has addressed formatting issues. One minor style issue remains: the comment "// Static files embedded in the binary" on line 27 should end with a period per godot linting standards.Update line 27 to:
// Static files embedded in the binary.web/index.html (1)
5-5: Update placeholder favicon.The favicon reference to
/vite.svgappears to be a default Vite placeholder. Consider replacing it with a project-specific icon for the Kube Bind application.web/.eslintrc.cjs (1)
14-16: Consider enabling multi-word component name enforcement.The
vue/multi-word-component-namesrule is disabled, which goes against the Vue style guide recommendation. This rule prevents naming conflicts with existing or future HTML elements. If intentional, consider documenting the rationale in a comment.web/README.md (1)
26-38: Clarify SPA vs API ownershipDocument that the SPA must not define routes under
/api/*to avoid breaking deep links and CLI/JSON responses. Frontend should use web-friendly paths (e.g.,/resources,/clusters/:cluster/resources) and only redirect to/api/*for auth flows. I see/api/*routes in the router; see my comment in web/src/main.ts.Would you like me to add a short “Routing Guarantees” section here?
web/vite.config.docker.ts (1)
39-42: Note: optimizeDeps affects dev, not build
optimizeDepsdoesn’t run invite build. Safe to keep, but it won’t influence production bundles.backend/options/options.go (3)
65-67: Fix comment name and clarify schemeThe comment mentions “ControllerFrontend” but the field is
Frontend. Also allowhttps://in the description.Apply:
-// If ControllerFrontend starts with http:// it is treated as a URL to a SPA server -// Else - it is treated as a path to static files to be served. +// If Frontend starts with http:// or https:// it is treated as a URL to an SPA server; +// otherwise it is treated as a filesystem path with static files to be served.
102-103: Default looks good; consider minimal validationDefaulting to
/wwwmatches the Docker image. Consider validating path existence (when not URL) during startup to fail fast.
147-147: Improve flag help and accept https://Update help text and add basic normalization/validation.
-fs.StringVar(&options.Frontend, "frontend", options.Frontend, "If starts with http:// it is treated as a URL to a SPA server Else - it is treated as a path to static files to be served.") +fs.StringVar(&options.Frontend, "frontend", options.Frontend, "If starts with http:// or https:// it is treated as a URL to an SPA server; otherwise, it is treated as a filesystem path with static assets to serve.")Optionally in
Validate():+ if strings.HasPrefix(options.Frontend, "http://") || strings.HasPrefix(options.Frontend, "https://") { + if _, err := url.ParseRequestURI(options.Frontend); err != nil { + return fmt.Errorf("invalid --frontend URL: %v", err) + } + } else if options.Frontend != "" { + if _, err := os.Stat(options.Frontend); err != nil { + return fmt.Errorf("invalid --frontend path %q: %v", options.Frontend, err) + } + }Dockerfile (3)
52-56: Remove redundant npm install
npm ciinstalls from lockfile; runningnpm installbeforehand is unnecessary and risks mutating the lockfile.-RUN npm install - # Install dependencies with specific flags to handle optional deps and architecture issues RUN npm ci --prefer-offline --no-audit --no-fund --no-optional
68-69: Avoid apk cache in final imageUse
--no-cacheto reduce image size.-RUN apk --update add ca-certificates +RUN apk add --no-cache ca-certificates
25-31: Improve Go module cachingLeverage
go mod downloadbefore copying the full source for better layer caching.-# <- COPY go.mod and go.sum files to the workspace -COPY go.mod . -COPY go.sum . - -# COPY the source code as the last step -COPY . . +# Copy and download dependencies first for cache efficiency +COPY go.mod go.sum ./ +RUN go mod download + +# Then copy the rest +COPY . .web/src/main.ts (1)
30-59: Apply the same query builder to external redirectsUse the helper to preserve multi-valued params reliably.
-redirect: (to: RouteLocationGeneric) => { - const query = new URLSearchParams(to.query as Record<string, string>).toString() - const url = `/api/authorize${query ? '?' + query : ''}` +redirect: (to: RouteLocationNormalized) => { + const url = `/api/authorize${toQueryString(to.query as any)}` setTimeout(() => window.location.href = url, 0) return '/' }Repeat similarly for
/clusters/:cluster/authorizeand/callback.web/vite.config.ts (2)
5-11: Factor shared config to reduce drift
vite.config.tsandvite.config.docker.tsdiverge slightly (hash formats, minify, chunks). Consider extracting a smallvite.config.base.tsand import/extend it in both to keep parity.
12-20: Dev proxy: consider ws and rewrite for futureIf you later add SSE/WS endpoints, add
ws: true. If backend serves API under a prefix, arewritemay be needed. For now this is fine.web/src/views/Resources.vue (2)
621-647: Robust redirect param handling
responsemay already exist; preferappendsemantics to preserve existing values and handle arrays.- parsedUrl.searchParams.set('response', response.encodedResponse) + parsedUrl.searchParams.append('response', response.encodedResponse)
694-703: Avoid double loading on mount
watch(..., { immediate: true })already triggersloadResources(); calling it again inonMounted()duplicates requests.onMounted(() => { // Handle OAuth callback first, then load resources handleOAuthCallback() - loadResources() })backend/http/handler.go (1)
123-132: Consider graceful error handling instead of panic.Line 126 uses panic for a reverse proxy creation failure. While the comment indicates this is for development only, panicking in the server initialization could be problematic. Consider returning an error from NewHandler or AddRoutes instead.
Apply this approach:
if strings.HasPrefix(h.frontend, "http://") { spaserver, err := spaserver.NewSPAReverseProxyServer(h.frontend) if err != nil { // Log error and fall back to file server or return error from AddRoutes return fmt.Errorf("failed to create SPA reverse proxy server: %w", err) } mux.PathPrefix("/").Handler(spaserver) } else { fileSystem := http.Dir(h.frontend) mux.PathPrefix("/").Handler(spaserver.NewSPAFileServer(fileSystem)) }Note: This would require changing the AddRoutes signature to return an error.
web/src/services/auth.ts (3)
68-82: Consider adding actual session verification.The checkAuthentication method only validates that sessionID is non-empty but doesn't verify the session with the backend. While the comment indicates cookies are HttpOnly, consider making a lightweight API call (e.g., to
/api/healthzwith credentials) to confirm the session is still valid.async checkAuthentication(sessionID: string): Promise<boolean> { try { if (!sessionID) { return false } // Optional: verify session with backend const response = await axios.get('/api/healthz', { withCredentials: true // Include HttpOnly cookies }) return response.status === 200 } catch (error) { return false } }
119-127: Document limitation of client-side cookie clearing.The clearSessionCookie method attempts to clear cookies via JavaScript, but HttpOnly cookies (which the backend uses for sessions) cannot be cleared this way. Consider documenting this limitation and/or adding a logout endpoint on the backend that clears the session cookie server-side.
Add a backend logout endpoint and call it:
async logout(): Promise<void> { try { await axios.post('/api/logout', {}, { withCredentials: true }) } catch (error) { // Log but don't throw - best effort logout } this.sessionInfo = null this.clearSessionCookie() // Clear any non-HttpOnly cookies }
142-182: Consider refactoring the bindResourceWithSession signature.The method has 11 parameters, making it difficult to use and maintain. Consider accepting a structured options object instead.
Refactor to use an options object:
interface BindResourceOptions { group: string resource: string apiVersion: string clusterId?: string sessionId: string scope?: string kind?: string name?: string permissionClaims?: PermissionClaim[] customRequestName?: string namespaces?: Array<{ name: string }> } async bindResourceWithSession(options: BindResourceOptions): Promise<any> { const { group, resource, apiVersion, clusterId = '', sessionId, scope = 'Namespaced', kind = '', name = '', permissionClaims = [], customRequestName, namespaces = [] } = options // ... rest of implementation }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
.github/workflows/ci.yamlis excluded by!**/*.yaml.github/workflows/image.yamlis excluded by!**/*.yamlbackend/static/web/dist/assets/index.54024e19.cssis excluded by!**/dist/**backend/static/web/dist/assets/index.fdabfc06.jsis excluded by!**/dist/**backend/static/web/dist/index.htmlis excluded by!**/dist/**contrib/kcp/deploy/resources/apiexport-kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiresourceschema-apiserviceexporttemplates.kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiresourceschema-bindableresourcesrequests.kube-bind.io.yamlis excluded by!**/*.yamldeploy/charts/backend/crds/kube-bind.io_apiserviceexporttemplates.yamlis excluded by!**/*.yamldeploy/charts/backend/crds/kube-bind.io_bindableresourcesrequests.yamlis excluded by!**/*.yamldeploy/charts/backend/examples/values-local-development.yamlis excluded by!**/*.yamldeploy/crd/kube-bind.io_apiserviceexporttemplates.yamlis excluded by!**/*.yamldeploy/crd/kube-bind.io_bindableresourcesrequests.yamlis excluded by!**/*.yamlweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
.github/workflows/goreleaser.yml(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)Dockerfile.konnector(1 hunks)Makefile(1 hunks)README.md(1 hunks)backend/http/handler.go(10 hunks)backend/options/options.go(3 hunks)backend/server.go(2 hunks)backend/spaserver/spaserver.go(1 hunks)backend/static/embed.go(1 hunks)cli/pkg/kubectl/bind/plugin/bind.go(2 hunks)docs/content/setup/helm.md(1 hunks)docs/content/setup/kcp-setup.md(2 hunks)hack/run-frontend.sh(1 hunks)sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go(1 hunks)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go(1 hunks)sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go(1 hunks)web/.eslintrc.cjs(1 hunks)web/.npmrc(1 hunks)web/README.md(1 hunks)web/index.html(1 hunks)web/package.json(1 hunks)web/src/App.vue(1 hunks)web/src/main.ts(1 hunks)web/src/services/auth.ts(1 hunks)web/src/views/Resources.vue(1 hunks)web/tsconfig.json(1 hunks)web/tsconfig.node.json(1 hunks)web/vite.config.docker.ts(1 hunks)web/vite.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
cli/pkg/kubectl/bind/plugin/bind.go (1)
sdk/apis/kubebind/v1alpha2/register.go (1)
GroupVersion(35-35)
backend/http/handler.go (5)
backend/spaserver/spaserver.go (2)
NewSPAReverseProxyServer(79-88)NewSPAFileServer(33-38)backend/session/session.go (1)
State(29-34)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (4)
BindableResourcesRequest(143-168)BindingResponse(32-53)BindingResponseAuthentication(57-63)BindingResponseAuthenticationOAuth2CodeGrant(68-75)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (8)
APIServiceExportRequestResponse(66-79)NameObjectMeta(81-84)APIServiceExportRequestSpec(95-127)APIServiceExportRequestResource(137-143)PermissionClaim(206-213)Namespaces(129-135)GroupResource(182-198)Selector(154-162)sdk/apis/kubebind/v1alpha2/claimable_apis.go (2)
ResolveClaimableAPI(83-90)ClaimableAPIs(38-81)
web/src/main.ts (1)
backend/static/web/dist/assets/index.fdabfc06.js (5)
routes(11904-11949)Resources(11903-11903)router(11950-11953)app(11954-11954)createApp(6627-6649)
sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (2)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (3)
BindableResource(89-138)BindableResourcesRequest(143-168)BindableResourcesResponse(77-86)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
PermissionClaim(206-213)Namespaces(129-135)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (2)
web/src/services/auth.ts (2)
BindableResource(11-19)PermissionClaim(45-52)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (1)
Namespaces(129-135)
backend/server.go (2)
backend/options/options.go (1)
Options(33-40)pkg/konnector/options/options.go (1)
Options(29-33)
web/src/services/auth.ts (2)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (2)
BindableResource(89-138)BindableResourcesRequest(143-168)backend/static/web/dist/assets/index.fdabfc06.js (1)
cookies(9866-9896)
🪛 golangci-lint (2.5.0)
backend/static/embed.go
[error] 25-25: File is not properly formatted
(gci)
[error] 29-29: Comment should end in a period
(godot)
backend/spaserver/spaserver.go
[error] 58-58: File is not properly formatted
(gci)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: goreleaser
- GitHub Check: lint
- GitHub Check: go-test
🔇 Additional comments (26)
docs/content/setup/helm.md (1)
53-57: Documentation update for development version deployment is properly formatted.The helm upgrade command correctly specifies a full git SHA (
0.0.0-fadb9edd26c0202f4a9511ee9d71b9e5f43672b9) in the version field, which improves reproducibility compared to short SHAs. The command syntax and formatting are consistent with the recommended approach shown in lines 45–50.Please confirm that the git SHA
fadb9edd26c0202f4a9511ee9d71b9e5f43672b9corresponds to a valid commit on the main branch and represents the intended development build for this PR..gitignore (1)
18-24: LGTM! Standard frontend artifact patterns.The ignore patterns are appropriate for the Vue.js/Vite frontend being introduced. They properly exclude build artifacts, dependencies, and tooling files from version control.
.github/workflows/goreleaser.yml (1)
20-20: LGTM! Checkout action upgrade.Upgrading to
actions/checkout@v4is appropriate and aligns with current GitHub Actions best practices.README.md (1)
61-61: LGTM! Documentation cross-reference.The reference to the frontend documentation is helpful for users working with the new Vue.js frontend.
web/tsconfig.node.json (1)
1-11: LGTM! Standard Vite Node configuration.The TypeScript configuration for the Vite build tooling is appropriate, with strict mode enabled and proper module resolution for the bundler.
docs/content/setup/kcp-setup.md (2)
92-92: LGTM! Backend configuration update.The addition of the
--consumer-scope=clusterflag is properly documented and aligns with the backend API changes in this PR.
136-139: LGTM! Additional setup steps documented.The new template and catalog resource creation steps are clearly documented and enhance the setup workflow.
web/src/App.vue (1)
1-133: LGTM! Clean Vue app shell.The component provides a solid foundation for the SPA with:
- Semantic HTML structure
- Comprehensive design system styles
- Proper router integration
The global styles establish consistent UI patterns across the application.
web/tsconfig.json (1)
1-31: LGTM!The TypeScript configuration is well-structured with strict type checking enabled and appropriate compiler options for a Vue 3 + Vite project. The path alias setup and project references are correctly configured.
backend/server.go (2)
68-71: LGTM!The callback URL update to include the
/apiprefix aligns with the API-first architecture introduced in this PR. The change maintains backward compatibility with the conditional default URL logic.
110-122: LGTM!The addition of the
Frontendparameter to theNewHandlercall enables the handler to serve the SPA and coordinate frontend routing, which is essential for the API-first refactoring.Dockerfile.konnector (1)
30-30: Alpine version 3.22.1 exists and is valid.The Docker Hub API confirms that Alpine version
3.22.1is an available and valid Docker image. The review comment's concern that this version may not exist is unfounded. The manifest inspect command failure in the verification script reflects sandbox environment limitations, not the non-existence of the image.Likely an incorrect or invalid review comment.
web/vite.config.docker.ts (1)
5-12: Config is correctly specified in build scriptVerification confirms the build script in package.json explicitly passes
--config vite.config.docker.tsto Vite. The Docker-specific configuration will be used during image builds.Dockerfile (1)
64-66: No changes required to Dockerfile.The review comment is incorrect. The npm
buildscript inweb/package.jsonalready explicitly specifies the Docker config:"build": "vite build --config vite.config.docker.ts". Runningnpm run buildin the Dockerfile executes this script as-is, which means the Docker-specific configuration is already being applied explicitly. Adding--config vite.config.docker.tsagain would be redundant and unnecessary.Likely an incorrect or invalid review comment.
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
77-168: LGTM! Types are well-structured and consistent.The new binding resource types are properly defined with appropriate kubebuilder validation markers. The types align with the frontend TypeScript interfaces in web/src/services/auth.ts, ensuring type safety across the stack.
backend/spaserver/spaserver.go (2)
28-38: LGTM! Clean constructor pattern.The SPAFileServer struct and constructor follow a clean design pattern, properly encapsulating the file system and delegating to http.FileServer.
74-92: LGTM! Simple and effective reverse proxy implementation.The SPAReverseProxyServer provides a clean wrapper for development-time proxying to a frontend server.
sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (1)
738-809: LGTM! Auto-generated DeepCopy methods.The DeepCopy methods for the new binding resource types are properly generated and follow the standard Kubernetes code generation patterns.
backend/http/handler.go (6)
68-68: LGTM! Clean integration of frontend configuration.The frontend field is properly added to the handler struct and initialized through the constructor.
Also applies to: 78-78, 87-87
135-155: LGTM! Clean redirect handler for CLI compatibility.The handleExportsRedirect properly redirects legacy CLI requests to the new /api endpoints while preserving query parameters.
399-447: LGTM! Clean migration to JSON-only response.The handleResources method correctly outputs JSON instead of HTML, aligning with the API-first architecture.
550-596: Verify session and request validation completeness.The session retrieval and basic validation look correct. However, ensure that the sessionID parameter is properly validated against the cookie name to prevent session fixation attacks.
Consider adding explicit validation:
sessionID := r.URL.Query().Get("s") if sessionID == "" { logger.Error(errors.New("missing session parameter"), "failed to get session from query") http.Error(w, "missing session parameter 's'", http.StatusBadRequest) return } + +// Validate sessionID format to prevent injection +if !isValidSessionID(sessionID) { + logger.Error(errors.New("invalid session ID format"), "validation failed") + http.Error(w, "invalid session ID format", http.StatusBadRequest) + return +}
698-719: Verify intended exposure of encoded response in JSON.The JSON response includes
encodedResponsewhich contains the base64-encoded binding response with kubeconfig credentials. Ensure this is the intended behavior for AJAX requests from the frontend, as it exposes sensitive credentials in the HTTP response body.If the frontend needs to pass this to the CLI, verify that:
- The frontend properly handles and protects this sensitive data
- The response is served over HTTPS in production
- The encoded response doesn't contain more sensitive data than necessary
746-758: LGTM! Simple and correct implementation.The handleBindableResources handler properly exposes the claimable APIs as JSON.
web/src/services/auth.ts (2)
3-63: LGTM! Type definitions are consistent with backend.The TypeScript interfaces properly mirror the Go types defined in the backend SDK, ensuring type safety across the full stack.
184-202: LGTM! Clean API methods with proper cluster-aware routing.Both getExports and getClaimableResources are properly implemented with appropriate error handling.
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
28a13a3 to
5f0cfbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (11)
web/README.md (1)
50-56: Fix flag typo and align Go version.The backend flag is
--frontend, not--frontent. Additionally, the Dockerfile uses Go 1.24, so the prerequisite should be updated to match.Apply:
-# Terminal 1: Start Go backend -go run ./cmd/backend --listen-port=8080 --frontent http://localhost:3000 +# Terminal 1: Start Go backend +go run ./cmd/backend --listen-port=8080 --frontend http://localhost:3000-- Go 1.19+ for running the backend server +- Go 1.24+ for running the backend serverDockerfile (1)
32-39: Verify cross-compile variables are passed to make.The
elsebranch setsGOOSandGOARCHenvironment variables, but if the Makefile doesn't use these and instead derivesOS/ARCHfrom the builder host, the produced binary may have the wrong architecture.Consider passing the cross-compile variables explicitly to make:
else \ - GOOS=$TARGETOS GOARCH=$TARGETARCH make build; \ + make build OS=$TARGETOS ARCH=$TARGETARCH; \ fiAlternatively, replace the make call with an explicit
go buildinvocation that usesGOOSandGOARCH.web/package.json (1)
15-18: Update axios to address critical security vulnerabilities.The specified version of axios (^1.5.0) contains multiple HIGH severity vulnerabilities, including DoS and SSRF issues. Update to version 1.12.0 or later.
Apply:
"dependencies": { "vue": "^3.3.4", "vue-router": "^4.2.4", - "axios": "^1.5.0" + "axios": "^1.12.0" },Run
npm installand update the lockfile after making this change.hack/run-frontend.sh (1)
3-5: Correct misleading comments.The comments state this script "builds the Vue.js frontend and embeds it into the Go binary," but the script actually runs the development server (
npm run dev), which does not build or embed anything.Apply:
-# Build frontend script for kube-bind -# This script builds the Vue.js frontend and embeds it into the Go binary +# Development frontend script for kube-bind +# This script runs the Vue.js frontend development serverweb/src/main.ts (2)
3-16: Replace non-existent RouteLocationGeneric and handle query arrays.
RouteLocationGenericis not a public vue-router v4 type. Additionally, castingto.querytoRecord<string,string>loses array values.Apply:
-import type { RouteLocationGeneric } from 'vue-router' +import type { RouteLocationNormalized } from 'vue-router' + +const toQueryString = (q: Record<string, unknown>) => { + const sp = new URLSearchParams() + for (const [k, v] of Object.entries(q)) { + if (Array.isArray(v)) v.forEach(x => sp.append(k, String(x ?? ''))) + else if (v != null) sp.append(k, String(v)) + } + const s = sp.toString() + return s ? `?${s}` : '' +}Then update the redirect callbacks:
- redirect: (to: RouteLocationGeneric) => { - const query = new URLSearchParams(to.query as Record<string, string>).toString() - return `/resources${query ? '?' + query : ''}` - } + redirect: (to: RouteLocationNormalized) => `/resources${toQueryString(to.query)}`Apply similar changes to other redirect callbacks.
21-27: Do not claim /api/ paths in the SPA router.*Routing
/api/resourcesand/api/clusters/:cluster/resourcesin the SPA conflicts with backend JSON endpoints and breaks deep links on page refresh.Remove these routes:
// Main resources route { path: '/resources', component: Resources }, - // API routes that match backend endpoints - all serve the same resources view - { path: '/api/resources', component: Resources }, - { path: '/api/clusters/:cluster/resources', component: Resources, props: true }, - // Web-friendly cluster-aware routes { path: '/clusters/:cluster/resources', component: Resources, props: true },web/src/views/Resources.vue (2)
200-206: Disable Bind button when template has no resources.The Bind button can be clicked for templates with no resources, which may cause runtime errors. Add a check to disable the button when the template has no resources.
Apply:
:disabled="binding === template.Name || bindingSuccess === template.Name || !isValidName(resourceNames[template.Name] || '') + || template.Resources.length === 0"
584-592: Correct kind and apiVersion derivation.The
kindfield uses the plural resource name instead of the singular PascalCase Kind, andapiVersionuses a bare version string instead of thegroup/versionformat. Additionally, only the first version is used without guarding against empty arrays.Consider this adjustment:
const bindableResources = template.Resources.map(resource => ({ name: `${resource.resource}.${resource.group}`, - kind: resource.resource, + // TODO: Map resource.resource (plural) to proper Kind (singular PascalCase) + kind: resource.resource, scope: template.Scope, - apiVersion: resource.versions[0], + version: resource.versions[0] ?? '', + apiVersion: resource.group ? `${resource.group}/${resource.versions[0] ?? ''}` : resource.versions[0] ?? '', group: resource.group, resource: resource.resource, sessionID: currentSessionId })) + +if (bindableResources.length === 0 || bindableResources.some(r => !r.version)) { + throw new Error('Template has no valid resources to bind') +}Verify that the backend API expects this format and adjust accordingly.
backend/spaserver/spaserver.go (1)
58-59: Fix formatting issue.The if statement spanning lines 58-59 has improper indentation, causing a linter error.
Apply:
- if strings.Contains(path, "/assets/") || strings.Contains(path, "/api/") || - (strings.Contains(path, ".") && !strings.HasSuffix(path, "/")) { + if strings.Contains(path, "/assets/") || strings.Contains(path, "/api/") || + (strings.Contains(path, ".") && !strings.HasSuffix(path, "/")) {Run
gofmtorgcito ensure proper formatting.web/src/services/auth.ts (1)
114-116: Use cryptographically secure random for session ID generation.The
generateSessionIdmethod usesMath.random(), which is not cryptographically secure and could allow session prediction attacks. Usecrypto.getRandomValues()for session IDs used in OAuth flows.backend/http/handler.go (1)
606-619: Validate bindRequest.Name before use.Line 612 uses
bindRequest.Namedirectly from user input without validation. This value must conform to Kubernetes DNS-1123 subdomain naming rules (lowercase alphanumeric, hyphens, dots, max 253 characters) when used asObjectMeta.Name.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
.github/workflows/ci.yamlis excluded by!**/*.yaml.github/workflows/image.yamlis excluded by!**/*.yamlbackend/static/web/dist/assets/index.54024e19.cssis excluded by!**/dist/**backend/static/web/dist/assets/index.fdabfc06.jsis excluded by!**/dist/**backend/static/web/dist/index.htmlis excluded by!**/dist/**contrib/kcp/deploy/resources/apiexport-kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiresourceschema-apiserviceexporttemplates.kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiresourceschema-bindableresourcesrequests.kube-bind.io.yamlis excluded by!**/*.yamldeploy/charts/backend/crds/kube-bind.io_apiserviceexporttemplates.yamlis excluded by!**/*.yamldeploy/charts/backend/crds/kube-bind.io_bindableresourcesrequests.yamlis excluded by!**/*.yamldeploy/crd/kube-bind.io_apiserviceexporttemplates.yamlis excluded by!**/*.yamldeploy/crd/kube-bind.io_bindableresourcesrequests.yamlis excluded by!**/*.yamlweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
.github/workflows/goreleaser.yml(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)Dockerfile.konnector(1 hunks)Makefile(1 hunks)README.md(1 hunks)backend/http/handler.go(10 hunks)backend/options/options.go(3 hunks)backend/server.go(2 hunks)backend/spaserver/spaserver.go(1 hunks)cli/go.mod(1 hunks)cli/pkg/kubectl/bind/plugin/bind.go(2 hunks)docs/content/setup/helm.md(1 hunks)docs/content/setup/kcp-setup.md(2 hunks)hack/run-frontend.sh(1 hunks)sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go(1 hunks)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go(1 hunks)sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go(1 hunks)web/.eslintrc.cjs(1 hunks)web/.npmrc(1 hunks)web/README.md(1 hunks)web/index.html(1 hunks)web/package.json(1 hunks)web/src/App.vue(1 hunks)web/src/main.ts(1 hunks)web/src/services/auth.ts(1 hunks)web/src/views/Resources.vue(1 hunks)web/tsconfig.json(1 hunks)web/tsconfig.node.json(1 hunks)web/vite.config.docker.ts(1 hunks)web/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- web/tsconfig.json
- sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go
- web/.npmrc
- backend/options/options.go
- web/index.html
- cli/pkg/kubectl/bind/plugin/bind.go
- .github/workflows/goreleaser.yml
- docs/content/setup/kcp-setup.md
- .gitignore
- Dockerfile.konnector
- web/vite.config.docker.ts
- web/tsconfig.node.json
- web/src/App.vue
- web/.eslintrc.cjs
- Makefile
- docs/content/setup/helm.md
- README.md
- backend/server.go
🧰 Additional context used
🧬 Code graph analysis (5)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (3)
web/src/services/auth.ts (2)
BindableResource(11-19)PermissionClaim(45-52)cli/pkg/kubectl/bind/plugin/bind.go (1)
SessionID(318-324)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (1)
Namespaces(129-135)
sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (2)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (3)
BindableResource(89-138)BindableResourcesRequest(143-168)BindableResourcesResponse(77-86)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
PermissionClaim(206-213)Namespaces(129-135)
web/src/main.ts (1)
backend/static/web/dist/assets/index.fdabfc06.js (5)
routes(11904-11949)Resources(11903-11903)router(11950-11953)app(11954-11954)createApp(6627-6649)
web/src/services/auth.ts (2)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (2)
BindableResource(89-138)BindableResourcesRequest(143-168)backend/static/web/dist/assets/index.fdabfc06.js (1)
cookies(9866-9896)
backend/http/handler.go (4)
backend/spaserver/spaserver.go (2)
NewSPAReverseProxyServer(79-88)NewSPAFileServer(33-38)backend/session/session.go (1)
State(29-34)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (4)
BindableResourcesRequest(143-168)BindingResponse(32-53)BindingResponseAuthentication(57-63)BindingResponseAuthenticationOAuth2CodeGrant(68-75)sdk/apis/kubebind/v1alpha2/claimable_apis.go (2)
ResolveClaimableAPI(83-90)ClaimableAPIs(38-81)
🪛 markdownlint-cli2 (0.18.1)
web/README.md
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: verify
- GitHub Check: go-test-e2e
- GitHub Check: go-test
- GitHub Check: goreleaser
🔇 Additional comments (12)
cli/go.mod (1)
13-13: Verify the go-spew version choice; an unreleased pre-release is currently pinned.The go-spew dependency has been moved from indirect to direct require, which is correct given its direct usage in
cli/pkg/kubectl/bind/plugin/bind.go. However, the latest go-spew release is v1.1.1, released August 17, 2018. The pinned versionv1.1.2-0.20180830191138-d8f796af33ccis a pre-release/commit hash from August 30, 2018, and is not an official release. Confirm that this specific unreleased version is necessary, or consider updating to the latest official release.sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (3)
77-86: LGTM!The
BindableResourcesResponsetype definition is clean and well-documented, with appropriate validation ensuring at least one resource is present.
140-168: LGTM!The
BindableResourcesRequesttype is well-structured with appropriate validation. The reuse of existingPermissionClaimandNamespacestypes maintains consistency across the API.
88-138: Align TypeScript interface with Go struct definition.The Go struct
BindableResourceincludes an optionalDescriptionfield, but the corresponding TypeScript interface inweb/src/services/auth.tsomits this field. This inconsistency may cause the description to be dropped when the API response is consumed by the frontend. Verify whether this omission is intentional or an oversight and update the TypeScript interface to includedescription?: stringif it should be preserved.sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (3)
738-752: LGTM!The
BindableResourcedeep copy implementation correctly uses a shallow copy since all fields are primitive string types.
754-787: LGTM!The
BindableResourcesRequestdeep copy implementation correctly handles all fields. The use ofcopy()forResourcesandNamespacesslices is appropriate since they contain only value types, whilePermissionClaimsproperly uses per-elementDeepCopyInto()to handle nested pointer fields.
789-809: LGTM!The
BindableResourcesResponsedeep copy implementation correctly copies theResourcesslice usingcopy(), which is appropriate for the value-type elements.web/vite.config.ts (1)
1-44: Configuration looks good.The Vite configuration is properly set up for Vue 3 development with appropriate proxy settings for the backend API and build options for production.
backend/http/handler.go (4)
68-68: LGTM: Frontend integration field.The addition of the
frontendfield to the handler and its integration through the constructor aligns well with the SPA serving functionality introduced in this PR.Also applies to: 78-78, 87-87
135-155: LGTM: CLI redirect implementation.The redirect handler correctly preserves backward compatibility for CLI tools that can't execute JavaScript, while routing them to the new API endpoints. Query parameter preservation and appropriate use of 302 redirects are both correct.
399-447: LGTM: Conversion to JSON-based API.The refactoring from HTML template rendering to JSON encoding correctly supports the API-first architecture. Error handling and response structure are appropriate.
746-758: LGTM: Bindable resources endpoint.The implementation correctly exposes the claimable APIs as a JSON endpoint. The error handling and response formatting are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/http/handler.go (1)
294-301: Decode OAuth state with URL-safe base64 (current decode will fail).state was encoded with base64.URLEncoding; decoding with StdEncoding is incorrect and breaks the callback.
-decoded, err := base64.StdEncoding.DecodeString(state) +decoded, err := base64.URLEncoding.DecodeString(state)test/e2e/bind/happy-case_test.go (1)
728-734: Fix template selector mismatch and typo in simulateBrowser.“Bind again” passes serviceGVR.Resource while earlier steps use templateClassIdentifier (“bind-*”). Also, tempalteName is misspelled.
- go simulateBrowser(t, authURLCh, serviceGVR.Resource) + go simulateBrowser(t, authURLCh, templateClassIdentifier)-func simulateBrowser(t *testing.T, authURLCh chan string, tempalteName string) { +func simulateBrowser(t *testing.T, authURLCh chan string, templateName string) { @@ - t.Logf("Clicking template %s", tempalteName) - err = browser.Click("a." + tempalteName) + t.Logf("Clicking template %s", templateName) + err = browser.Click("a." + templateName)Also applies to: 743-756
♻️ Duplicate comments (6)
web/src/main.ts (2)
21-21: Replace RouteLocationGeneric with RouteLocationNormalized.The redirect callback parameter uses the non-public
RouteLocationGenerictype. Use the importedRouteLocationNormalizedtype instead.Apply this diff:
- redirect: (to: RouteLocationGeneric) => `/resources${toQueryString(to.query as any)}` + redirect: (to: RouteLocationNormalized) => `/resources${toQueryString(to.query as any)}`
3-3: Remove the non-public RouteLocationGeneric type import.The import includes
RouteLocationGeneric, which is not a public vue-router v4 type as previously flagged. AlthoughRouteLocationNormalizedis correctly imported,RouteLocationGenericis still being used on line 21.Apply this diff to remove the unused type:
-import type { RouteLocationNormalized, RouteLocationGeneric } from 'vue-router' +import type { RouteLocationNormalized } from 'vue-router'hack/run-frontend.sh (1)
3-5: Correct misleading comments about script purpose.The comments state "builds the Vue.js frontend and serves it using a development server," but the script executes
npm run devwhich runs a development server without performing a production build. The comments should accurately reflect that this is a development script.Apply this diff to correct the comments:
-# Build frontend script for kube-bind -# This script builds the Vue.js frontend and serves it using a development server. +# Development frontend script for kube-bind +# This script runs the Vue.js frontend development server for local development.backend/http/handler.go (3)
457-471: Don’t pass session/cookie name in query; derive cookie name from route.Reading the cookie name from ?s= leaks in URLs/logs and is brittle. Use generateCookieName with {cluster} param (or default).
- cookieName := r.URL.Query().Get("s") - ck, err := r.Cookie(r.URL.Query().Get("s")) + cookieName := generateCookieName(mux.Vars(r)["cluster"]) + ck, err := r.Cookie(cookieName) if err != nil { ... }Optionally keep ?s= as a backward-compat fallback if present. I can provide a patch if you prefer that path.
557-571: Same as above for POST /api/bind: avoid session in query; use deterministic cookie name.Mirror the GET handler behavior; prefer cookie by generateCookieName.
- // Get session ID from query parameter - sessionID := r.URL.Query().Get("s") - if sessionID == "" { - logger.Error(errors.New("missing session parameter"), "failed to get session from query") - http.Error(w, "missing session parameter 's'", http.StatusBadRequest) - return - } - - // Get session cookie - ck, err := r.Cookie(sessionID) + // Get session cookie using deterministic name + cookieName := generateCookieName(mux.Vars(r)["cluster"]) + ck, err := r.Cookie(cookieName) if err != nil { ... } - // Decode session state + // Decode session state state := session.State{} s := securecookie.New(h.cookieSigningKey, h.cookieEncryptionKey) - if err := s.Decode(sessionID, ck.Value, &state); err != nil { + if err := s.Decode(cookieName, ck.Value, &state); err != nil { ... }
622-635: Validate or generate export request name before use.bindRequest.Name is user input; validate per DNS-1123 rules or generate a safe default to avoid server-side create failures.
- ObjectMeta: kubebindv1alpha2.NameObjectMeta{ - Name: bindRequest.Name, - }, + // Validate name if provided; otherwise generate a safe default + name := bindRequest.Name + if name == "" { + name = fmt.Sprintf("bind-%d", time.Now().Unix()) + } else { + if errs := validation.IsDNS1123Subdomain(name); len(errs) > 0 { + http.Error(w, "invalid name: "+strings.Join(errs, "; "), http.StatusBadRequest) + return + } + } + ObjectMeta: kubebindv1alpha2.NameObjectMeta{ + Name: name, + },Add import:
import "k8s.io/apimachinery/pkg/util/validation"
🧹 Nitpick comments (9)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
112-116: Missing validation for Scope values.The comment indicates that Scope should be either "Cluster" or "Namespaced", but the
Validate()method does not enforce this constraint. Consider adding validation or a kubebuilder enum marker.Apply this diff to add validation in the
Validate()method:if res.Scope == "" { return fmt.Errorf("resource at index %d has no scope", i) } + if res.Scope != "Cluster" && res.Scope != "Namespaced" { + return fmt.Errorf("resource at index %d has invalid scope %q: must be either 'Cluster' or 'Namespaced'", i, res.Scope) + }web/embed.go (1)
30-37: Consider more graceful error handling instead of panic.The
GetFileSystem()function panics if creating the sub-filesystem fails. This is appropriate for initialization-time failures that indicate build/packaging errors. However, consider if a sentinel error or compile-time check would be more idiomatic.If you prefer avoiding runtime panics, consider returning an error:
-// GetFileSystem returns the embedded file system for serving static files. -func GetFileSystem() http.FileSystem { +// GetFileSystem returns the embedded file system for serving static files. +// Returns an error if the dist subdirectory cannot be accessed. +func GetFileSystem() (http.FileSystem, error) { // Create a sub-filesystem starting from dist subFS, err := fs.Sub(StaticFiles, "dist") if err != nil { - panic("failed to create sub filesystem: " + err.Error()) + return nil, fmt.Errorf("failed to create sub filesystem: %w", err) } - return http.FS(subFS) + return http.FS(subFS), nil }However, since this function is likely called during server initialization, the panic behavior may be acceptable for fail-fast semantics.
Dockerfile (3)
26-30: Remove redundant npm install; use npm ci with production flags.Running npm install followed by npm ci wastes time and cache. Prefer a single deterministic install.
-RUN npm install - -# Install dependencies with specific flags to handle optional deps and architecture issues -RUN npm ci --prefer-offline --no-audit --no-fund --no-optional +# Deterministic, production-only install +RUN npm ci --omit=dev --no-audit --no-fund --no-optional
74-75: Drop unused /www copy (assets are embedded) or wire the server to use it.With embedded FS via web.GetFileSystem(), /www is unused and adds size. Remove it unless you plan to serve from disk.
-COPY --from=ui-build-env /app/dist /www +# UI assets are embedded; skip copying to runtime image
71-71: Use --no-cache for apk in final image.Smaller, cleaner layer.
-RUN apk --update add ca-certificates +RUN apk add --no-cache ca-certificatesbackend/http/handler.go (2)
123-135: Support https frontend in dev and prefer scheme detection via url.Parse.Only http:// is recognized; https:// dev servers won’t be proxied.
-switch { -case strings.HasPrefix(h.frontend, "http://"): +switch u, err := url.Parse(h.frontend); { +case err == nil && (u.Scheme == "http" || u.Scheme == "https"): // Development mode: proxy to frontend dev server - spaserver, err := spaserver.NewSPAReverseProxyServer(h.frontend) + spaserver, err := spaserver.NewSPAReverseProxyServer(h.frontend) if err != nil { return err } mux.PathPrefix("/").Handler(spaserver) default: fs := web.GetFileSystem() mux.PathPrefix("/").Handler(spaserver.NewSPAFileServer(fs)) }
582-597: Close request body after reading.Wrap with MaxBytesReader but still defer close.
const maxBodySize = 1 << 20 // 1 MB r.Body = http.MaxBytesReader(w, r.Body, maxBodySize) +defer r.Body.Close() var bindRequest kubebindv1alpha2.BindableResourcesRequest if err := json.NewDecoder(r.Body).Decode(&bindRequest); err != nil {test/e2e/bind/happy-case_test.go (2)
132-146: Prefer /api/exports consistently (redirect exists but avoid reliance).Current tests mix /exports and /api/exports. Use /api/exports to reduce redirect coupling.
- fmt.Sprintf("http://%s/exports", addr.String()) + fmt.Sprintf("http://%s/api/exports", addr.String())Also applies to: 730-734
66-68: Fix misleading comment about cleanup.Comment says “Commented out …” but t.Cleanup(cancel) is active. Update or remove the comment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/ci.yamlis excluded by!**/*.yamlhack/dex-config-dev.yamlis excluded by!**/*.yaml
📒 Files selected for processing (20)
Dockerfile(1 hunks)Makefile(1 hunks)backend/http/handler.go(11 hunks)backend/server.go(2 hunks)backend/spaserver/spaserver.go(1 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)contrib/kcp/README.md(3 hunks)docs/content/setup/quickstart.md(1 hunks)go.mod(2 hunks)hack/run-frontend.sh(1 hunks)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go(2 hunks)test/e2e/bind/happy-case_test.go(7 hunks)test/e2e/framework/backend.go(1 hunks)test/e2e/framework/dex.go(1 hunks)web/README.md(1 hunks)web/embed.go(1 hunks)web/go.mod(1 hunks)web/src/main.ts(1 hunks)web/src/services/auth.ts(1 hunks)web/src/views/Resources.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/go.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/pkg/kubectl/bind/plugin/bind.go
- web/src/services/auth.ts
- web/README.md
- backend/server.go
- web/src/views/Resources.vue
🧰 Additional context used
🧬 Code graph analysis (3)
test/e2e/bind/happy-case_test.go (1)
test/e2e/framework/bind.go (1)
Bind(36-75)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (3)
web/src/services/auth.ts (3)
BindableResource(11-19)PermissionClaim(45-52)BindableResourcesRequest(54-63)cli/pkg/kubectl/bind/plugin/bind.go (1)
SessionID(317-323)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (1)
Namespaces(129-135)
backend/http/handler.go (6)
backend/spaserver/spaserver.go (2)
NewSPAReverseProxyServer(116-125)NewSPAFileServer(33-38)web/embed.go (1)
GetFileSystem(31-38)backend/session/session.go (1)
State(29-34)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (4)
BindableResourcesRequest(148-173)BindingResponse(37-58)BindingResponseAuthentication(62-68)BindingResponseAuthenticationOAuth2CodeGrant(73-80)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (7)
APIServiceExportRequestResponse(66-79)NameObjectMeta(81-84)APIServiceExportRequestSpec(95-127)APIServiceExportRequestResource(137-143)PermissionClaim(206-213)Namespaces(129-135)GroupResource(182-198)sdk/apis/kubebind/v1alpha2/claimable_apis.go (2)
ResolveClaimableAPI(83-90)ClaimableAPIs(38-81)
🪛 GitHub Check: go-test
web/embed.go
[failure] 27-27:
pattern dist/*: no matching files found
[failure] 27-27:
pattern dist/*: no matching files found
[failure] 27-27:
pattern dist/*: no matching files found
[failure] 27-27:
pattern dist/*: no matching files found
🪛 golangci-lint (2.5.0)
backend/http/handler.go
[error] 43-43: could not import github.com/kube-bind/kube-bind/web (web/embed.go:27:12: pattern dist/*: no matching files found)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: goreleaser
🔇 Additional comments (15)
web/src/main.ts (2)
7-15: Well-implemented query string serialization.The helper correctly handles array values and null checks, preserving all query parameters during redirects as intended.
27-34: Standard Vue 3 application bootstrap.The router creation with web history mode and app initialization follow Vue 3 best practices.
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (2)
82-91: LGTM!The type definition follows Kubernetes API conventions with appropriate validation markers.
145-173: LGTM!The type definition is well-structured with appropriate validation markers and documentation.
docs/content/setup/quickstart.md (1)
89-89: LGTM! Documentation updated to reflect API path migration.The OIDC callback URL is correctly updated to
/api/callback, consistent with the backend API path refactoring across the PR.go.mod (1)
9-9: LGTM! Web module replacement correctly configured.The local module replacement for the web component is properly set up for the monorepo structure.
contrib/kcp/README.md (2)
58-58: LGTM! KCP documentation updated for API path migration.The OIDC callback URL is correctly updated to
/api/callback.
111-111: Verify the cluster ID changes are intentional.The LogicalCluster URL reference and kubectl-bind exports path have been updated with different cluster IDs. Please confirm these are example placeholders and not hardcoded values that should be dynamic.
Also applies to: 127-127
hack/run-frontend.sh (1)
6-21: LGTM! Script logic is sound.The script properly checks for node_modules and installs dependencies only when needed before starting the development server.
Makefile (2)
378-380: LGTM! Frontend build target is correctly implemented.The
build-webtarget appropriately navigates to the web directory and runs the npm build command.
416-424: LGTM! Repository variable consistency resolved.The introduction of
DOCKER_REPOdefaulting toIMAGE_REPOand its usage inkind-loadresolves the previous mismatch issue.test/e2e/framework/backend.go (1)
86-86: LGTM! Test framework updated for API path migration.The callback URL construction is correctly updated to
/api/callback, ensuring test framework alignment with the backend API changes.test/e2e/framework/dex.go (1)
107-107: LGTM! Dex redirect URI updated correctly.The OAuth redirect URI is properly updated to
/api/callback, maintaining consistency with the API path refactoring.web/embed.go (1)
27-28: Build order is correctly configured.The Dockerfile properly sequences the frontend build before the Go binary compilation. The
npm run buildcommand executes at line 39 in the builder stage, while the Go build stage begins at line 42, ensuring thedistdirectory exists when the//go:embed dist/*directive is processed. The explanation in the review comment is accurate.Dockerfile (1)
24-24: No credential leakage risk detected — .npmrc contains only configuration flags.The
.npmrcfile at./web/.npmrccontains only npm configuration settings (optional=false,fund=false,audit=false) with no authentication tokens or credentials. The verification condition specified in the review comment ("if yes") is not met, and the code is acceptable as-is from a credential security perspective.
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
2 times, most recently
from
3cdbbf9 to
6d5d903
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/pkg/kubectl/bind/cmd/cmd.go (1)
103-103: Correct the nil parameter passed to Run method.The
Runmethod signature expectschan<- stringas the second parameter, but the code passesnil. Line 103 incli/pkg/kubectl/bind/cmd/cmd.gomust be corrected to pass a valid send-only string channel instead ofnil.Reference:
cli/pkg/kubectl/bind/plugin/bind.go:337shows the method signaturefunc (b *BindOptions) Run(ctx context.Context, urlCh chan<- string) error.contrib/kcp/test/e2e/kcp.go (1)
40-49: Do not mutate the shared REST config in-place.
cfg := restkeeps the original pointer, so the subsequent host rewrite changes the caller’s config. After the first invocation every later client ends up with/clusters/.../clusters/..., breaking requests and racing with other users of the base config. Please work on a copy before adjusting the host.-func wsConfig(t testing.TB, rest *rest.Config, workspace logicalcluster.Path) (*rest.Config, string) { - cfg := rest - cfg.Host += "/clusters/" + workspace.String() +func wsConfig(t testing.TB, base *rest.Config, workspace logicalcluster.Path) (*rest.Config, string) { + cfg := rest.CopyConfig(base) + cfg.Host = strings.TrimSuffix(base.Host, "/") + "/clusters/" + workspace.String()
♻️ Duplicate comments (3)
web/README.md (1)
43-45: Update Go prerequisite to match the current toolchain.The backend builds against Go 1.24 (see Dockerfile), so keeping “Go 1.19+” here misleads contributors and will lead to failed local builds. Please align the prerequisite with the actual minimum version.
-- Go 1.19+ for running the backend server +- Go 1.24+ for running the backend serverDockerfile (1)
59-66: Fix embedded asset copy path to prevent build failure.
web/embed.gouses//go:embed dist/*, which resolves toweb/dist/*at build time. Copying the Vue build output into./backend/static/web/distleavesweb/distempty, sogo buildfails with “pattern dist/*: no matching files found” and the binary misses the SPA assets. Copy the files into./web/distbefore running the Go build.-# Copy built UI assets for embedding -COPY --from=ui-build-env /app/dist ./backend/static/web/dist +# Copy built UI assets for embedding +COPY --from=ui-build-env /app/dist ./web/distbackend/http/handler.go (1)
333-341: Validate bindable request name before serializing it.
bindRequest.Namestill flows straight intoObjectMeta.Name, so an empty or non-DNS1123 value from the client will bubble up as a late failure when the CLI posts the request to Kubernetes. Please reject invalid names here (e.g. ensure non-empty and usevalidation.IsDNS1123Subdomain), returninghttp.StatusBadRequeston failure, and only populate the object when validation passes. Remember to add thek8s.io/apimachinery/pkg/util/validationimport.- request := kubebindv1alpha2.APIServiceExportRequestResponse{ + if bindRequest.Name == "" { + http.Error(w, "name is required", http.StatusBadRequest) + return + } + if errs := validation.IsDNS1123Subdomain(bindRequest.Name); len(errs) > 0 { + http.Error(w, "invalid name: "+strings.Join(errs, ", "), http.StatusBadRequest) + return + } + + request := kubebindv1alpha2.APIServiceExportRequestResponse{ TypeMeta: metav1.TypeMeta{ APIVersion: kubebindv1alpha2.SchemeGroupVersion.String(), Kind: "APIServiceExportRequest", },
🧹 Nitpick comments (7)
test/e2e/framework/apply.go (1)
91-95: LGTM! The[]bytecase is implemented correctly.The logic correctly unmarshals YAML from a byte slice and integrates seamlessly with existing manifest handling.
Optionally, the
stringand[]bytecases share identical unmarshal logic. You could consolidate them, though the current duplication is minimal and acceptable for test utilities.test/e2e/framework/kcp.go (1)
66-71: Consider consolidating withWithNameto simplify the API surface.The new
WithStaticNamefunction provides the same behavior as callingWithName("some-name")without format arguments. While the explicit naming may offer semantic clarity, maintaining both functions increases the API surface and may introduce confusion about which to use.If the intent is to signal non-formatted names explicitly, the current implementation is acceptable. Otherwise,
WithNamealready covers this use case.test/e2e/framework/browser.go (1)
41-48: Consider adding authentication completion verification.The function opens the authentication URL but does not verify that the authentication flow completes successfully. If the test expects authentication to finish, consider using
BrowserEventuallyAtPathor adding a mechanism to verify the authentication callback completes.Example enhancement:
func SimulateBrowser(t *testing.T, authURLCh chan string) { browser := surf.NewBrowser() authURL := <-authURLCh t.Logf("Browsing to auth URL: %s", authURL) err := browser.Open(authURL) require.NoError(t, err) + + // Wait for authentication to complete (adjust path as needed) + BrowserEventuallyAtPath(t, browser, "/callback") }web/vite.config.docker.ts (1)
19-20: Reconsider disabling minification.Disabling minification significantly increases bundle size. Minification (terser/esbuild) typically does not involve native dependencies—the native dependency issues are addressed by excluding the Rollup native bundles (lines 40-41). Consider re-enabling minification for production builds.
Apply this diff to enable minification:
- // Disable minification to avoid potential native dep issues - minify: false, + // Use esbuild for minification (no native deps) + minify: 'esbuild',contrib/kcp/test/e2e/backend.go (1)
42-42: Validate the host extraction logic.The code assumes
rest.Hostcontains/clusters/and uses string splitting to extract the base host. If the format differs, the extraction may produce incorrect results. Add validation to ensure the expected structure.Apply this diff to add validation:
- rest.Host = strings.Split(rest.Host, "/clusters/")[0] + parts := strings.Split(rest.Host, "/clusters/") + if len(parts) < 2 { + t.Fatalf("Unexpected rest.Host format: %s (expected format with /clusters/)", rest.Host) + } + rest.Host = parts[0]cli/pkg/kubectl/bind-templates/cmd/cmd.go (1)
61-68: Cap positional args at one server URL.Today a user can pass multiple positional URLs (all starting with http/https) and only the first one would be consumed, silently ignoring the rest. Please hard-limit the positional args to at most one while keeping the scheme validation.
Args: func(cmd *cobra.Command, args []string) error { + if len(args) > 1 { + return fmt.Errorf("expected at most one server URL, got %d", len(args)) + } for _, arg := range args { if !strings.HasPrefix(arg, "http://") && !strings.HasPrefix(arg, "https://") { return fmt.Errorf("invalid server URL: %s", arg) } } return nilcli/pkg/kubectl/base/client.go (1)
52-64: Preserve client creation error context.Line 63 replaces the original
client.NewClienterror with a generic “not authenticated” message, making real failures (e.g., corrupt config, missing server definition) hard to diagnose. Wrap and return the original error instead so downstream callers and users understand what actually failed.- return nil, s.URL, fmt.Errorf("not authenticated to server: %s", o.Server) + return nil, s.URL, fmt.Errorf("failed to create authenticated client: %w", err)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
.github/workflows/ci.yamlis excluded by!**/*.yaml.github/workflows/image.yamlis excluded by!**/*.yaml.golangci.yamlis excluded by!**/*.yamlcertficate.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiexport-kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiresourceschema-apiserviceexporttemplates.kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/deploy/resources/apiresourceschema-bindableresourcesrequests.kube-bind.io.yamlis excluded by!**/*.yamlcontrib/kcp/go.sumis excluded by!**/*.sumdeploy/charts/backend/crds/kube-bind.io_apiserviceexporttemplates.yamlis excluded by!**/*.yamldeploy/charts/backend/crds/kube-bind.io_bindableresourcesrequests.yamlis excluded by!**/*.yamldeploy/crd/kube-bind.io_apiserviceexporttemplates.yamlis excluded by!**/*.yamldeploy/crd/kube-bind.io_bindableresourcesrequests.yamlis excluded by!**/*.yamlgo.sumis excluded by!**/*.sumhack/dex-config-dev.yamlis excluded by!**/*.yamlweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (85)
.github/workflows/goreleaser.yml(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)Dockerfile.konnector(1 hunks)Makefile(4 hunks)README.md(1 hunks)backend/auth/handler.go(1 hunks)backend/auth/jwt.go(1 hunks)backend/auth/middleware.go(1 hunks)backend/auth/types.go(2 hunks)backend/client/client.go(1 hunks)backend/http/handler.go(8 hunks)backend/kubernetes/manager.go(1 hunks)backend/options/options.go(3 hunks)backend/server.go(4 hunks)backend/spaserver/spaserver.go(1 hunks)cli/cmd/kubectl-bind/cmd/kubectlBind.go(2 hunks)cli/go.mod(1 hunks)cli/pkg/client/client.go(1 hunks)cli/pkg/kubectl/base/browser.go(1 hunks)cli/pkg/kubectl/base/client.go(1 hunks)cli/pkg/kubectl/base/options.go(4 hunks)cli/pkg/kubectl/base/printer.go(1 hunks)cli/pkg/kubectl/bind-apiservice/plugin/bind.go(6 hunks)cli/pkg/kubectl/bind-apiservice/plugin/binder.go(1 hunks)cli/pkg/kubectl/bind-apiservice/plugin/konnector.go(3 hunks)cli/pkg/kubectl/bind-apiservice/plugin/secret.go(2 hunks)cli/pkg/kubectl/bind-apiservice/plugin/servicebindings.go(2 hunks)cli/pkg/kubectl/bind-collections/cmd/cmd.go(1 hunks)cli/pkg/kubectl/bind-collections/plugin/collections.go(1 hunks)cli/pkg/kubectl/bind-login/cmd/cmd.go(1 hunks)cli/pkg/kubectl/bind-login/plugin/login.go(1 hunks)cli/pkg/kubectl/bind-templates/cmd/cmd.go(1 hunks)cli/pkg/kubectl/bind-templates/plugin/templates.go(1 hunks)cli/pkg/kubectl/bind/cmd/cmd.go(1 hunks)cli/pkg/kubectl/bind/plugin/authenticate.go(1 hunks)cli/pkg/kubectl/bind/plugin/bind.go(5 hunks)cli/pkg/kubectl/bind/plugin/flags.go(0 hunks)cli/pkg/kubectl/bind/plugin/flags_test.go(0 hunks)cli/pkg/kubectl/config/config.go(1 hunks)contrib/kcp/README.md(3 hunks)contrib/kcp/deploy/bootstrap.go(2 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/backend.go(2 hunks)contrib/kcp/test/e2e/binding.go(1 hunks)contrib/kcp/test/e2e/browser.go(0 hunks)contrib/kcp/test/e2e/kcp.go(2 hunks)contrib/kcp/test/e2e/kcp_test.go(3 hunks)docs/content/setup/.pages(1 hunks)docs/content/setup/helm.md(1 hunks)docs/content/setup/kcp-setup.md(2 hunks)docs/content/setup/kubectl-plugin.md(1 hunks)docs/content/setup/quickstart.md(1 hunks)go.mod(3 hunks)hack/run-frontend.sh(1 hunks)sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go(2 hunks)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go(2 hunks)sdk/apis/kubebind/v1alpha2/register.go(1 hunks)sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go(4 hunks)test/e2e/bind/happy-case_test.go(4 hunks)test/e2e/framework/apply.go(1 hunks)test/e2e/framework/backend.go(1 hunks)test/e2e/framework/bind.go(1 hunks)test/e2e/framework/browser.go(2 hunks)test/e2e/framework/dex.go(2 hunks)test/e2e/framework/kcp.go(1 hunks)web/.eslintrc.cjs(1 hunks)web/.npmrc(1 hunks)web/README.md(1 hunks)web/embed.go(1 hunks)web/go.mod(1 hunks)web/index.html(1 hunks)web/package.json(1 hunks)web/src/App.vue(1 hunks)web/src/components/BindingResult.vue(1 hunks)web/src/components/TemplateBindingModal.vue(1 hunks)web/src/main.ts(1 hunks)web/src/services/auth.ts(1 hunks)web/src/services/http.ts(1 hunks)web/src/types/binding.ts(1 hunks)web/src/views/Resources.vue(1 hunks)web/tsconfig.json(1 hunks)web/tsconfig.node.json(1 hunks)web/vite.config.docker.ts(1 hunks)web/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- cli/pkg/kubectl/bind/plugin/flags_test.go
- contrib/kcp/test/e2e/browser.go
- cli/pkg/kubectl/bind/plugin/flags.go
✅ Files skipped from review due to trivial changes (6)
- web/tsconfig.json
- web/tsconfig.node.json
- docs/content/setup/kubectl-plugin.md
- cli/pkg/kubectl/bind-apiservice/plugin/servicebindings.go
- cli/pkg/kubectl/bind-apiservice/plugin/secret.go
- Makefile
🚧 Files skipped from review as they are similar to previous changes (19)
- cli/go.mod
- test/e2e/framework/backend.go
- backend/options/options.go
- docs/content/setup/quickstart.md
- web/src/App.vue
- web/index.html
- .github/workflows/goreleaser.yml
- web/src/services/auth.ts
- web/.npmrc
- docs/content/setup/helm.md
- hack/run-frontend.sh
- backend/spaserver/spaserver.go
- web/vite.config.ts
- .gitignore
- web/src/main.ts
- contrib/kcp/README.md
- README.md
- web/package.json
- web/go.mod
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-09-12T09:05:29.762Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.762Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
web/src/views/Resources.vuetest/e2e/bind/happy-case_test.gosdk/apis/kubebind/v1alpha2/bindingresponse_types.gobackend/http/handler.gobackend/auth/types.go
📚 Learning: 2025-09-18T16:39:25.771Z
Learnt from: xrstf
Repo: kube-bind/kube-bind PR: 0
File: :0-0
Timestamp: 2025-09-18T16:39:25.771Z
Learning: The lister-gen issue with singular vs plural resource names is an upstream bug in kubernetes/code-generator that has existed for 8+ years since inception. The bug is located in lister.go line 304 where singular resource names are incorrectly used instead of plural names, affecting any project using lister-gen.
Applied to files:
web/src/views/Resources.vue
📚 Learning: 2025-09-22T13:32:29.499Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: pkg/konnector/controllers/cluster/claimedresources/claimedresources_controller.go:255-263
Timestamp: 2025-09-22T13:32:29.499Z
Learning: In kube-bind's claimedresources controller (pkg/konnector/controllers/cluster/claimedresources/claimedresources_controller.go), the controller does not run if apiServiceExport is not set. This means nil-checks for c.apiServiceExport are unnecessary since the controller lifecycle ensures it's always non-nil when active.
Applied to files:
test/e2e/bind/happy-case_test.gosdk/apis/kubebind/v1alpha2/bindingresponse_types.gocli/pkg/kubectl/bind/plugin/bind.gocli/pkg/kubectl/bind-apiservice/plugin/binder.gobackend/http/handler.gobackend/server.go
📚 Learning: 2025-09-22T13:20:49.952Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: sdk/apis/kubebind/v1alpha2/boundchema_types.go:49-0
Timestamp: 2025-09-22T13:20:49.952Z
Learning: In kube-bind, there are two different ResourceGroupName() methods: BoundSchema.ResourceGroupName() for CRDs (always non-empty groups) uses simple fmt.Sprintf formatting, while APIServiceExportRequestResource.ResourceGroupName() for export requests handles empty groups by converting to "core". BoundSchema is exclusively for CRDs which cannot have empty API groups per Kubernetes validation.
Applied to files:
test/e2e/bind/happy-case_test.gosdk/apis/kubebind/v1alpha2/bindingresponse_types.gosdk/apis/kubebind/v1alpha2/register.gobackend/http/handler.go
📚 Learning: 2025-09-22T13:20:49.952Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: sdk/apis/kubebind/v1alpha2/boundchema_types.go:49-0
Timestamp: 2025-09-22T13:20:49.952Z
Learning: In kube-bind, BoundSchema.ResourceGroupName() is only used for CRDs (Custom Resource Definitions), and CRDs must always have non-empty API groups. Therefore, handling empty groups (core API group "") is not necessary in this context, unlike general Kubernetes GroupResource handling.
Applied to files:
test/e2e/bind/happy-case_test.gosdk/apis/kubebind/v1alpha2/bindingresponse_types.gosdk/apis/kubebind/v1alpha2/register.gobackend/http/handler.gobackend/auth/types.go
📚 Learning: 2025-10-22T06:41:37.475Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 342
File: docs/content/usage/migration.md:34-37
Timestamp: 2025-10-22T06:41:37.475Z
Learning: In the kube-bind repository documentation and examples, "mangodb" (with group "mangodb.com" and resource "mangodbs") is an intentional fictional example name chosen to avoid referencing real products like MongoDB. It should not be treated as a typo.
Applied to files:
test/e2e/bind/happy-case_test.gobackend/auth/types.go
📚 Learning: 2025-09-19T05:56:35.969Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: backend/controllers/servicenamespace/servicenamespace_reconcile.go:81-98
Timestamp: 2025-09-19T05:56:35.969Z
Learning: In kube-bind, RBAC permissions for PermissionClaims use "*" verbs intentionally. This is a design decision based on: 1) permissions are scoped to consumer-owned provider namespaces, limiting blast radius, 2) bidirectional resource flow requires broad permissions for operations like initial resource creation from consumer side, 3) kube-bind's architecture prioritizes operational simplicity over granular RBAC within the namespace security boundary.
Applied to files:
test/e2e/bind/happy-case_test.gosdk/apis/kubebind/v1alpha2/bindingresponse_types.gocli/pkg/kubectl/bind-apiservice/plugin/bind.gobackend/http/handler.gobackend/auth/types.go
📚 Learning: 2025-09-12T08:40:15.290Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.290Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
sdk/apis/kubebind/v1alpha2/bindingresponse_types.goweb/src/types/binding.tsbackend/http/handler.go
📚 Learning: 2025-09-19T06:28:44.853Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go:148-150
Timestamp: 2025-09-19T06:28:44.853Z
Learning: In kube-bind, permission claims must all have the same scope (either all cluster-scoped or all namespace-scoped). Mixed scopes are not allowed, which means the isClusterScoped flag can be safely determined from any processed schema in the export.
Applied to files:
sdk/apis/kubebind/v1alpha2/bindingresponse_types.gobackend/http/handler.gobackend/auth/types.gocontrib/kcp/test/e2e/kcp_test.go
📚 Learning: 2025-09-12T08:55:41.860Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.860Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
sdk/apis/kubebind/v1alpha2/bindingresponse_types.gobackend/http/handler.gobackend/auth/types.go
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
Repo: kube-bind/kube-bind PR: 333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/backend.gocontrib/kcp/test/e2e/kcp.gocontrib/kcp/test/e2e/kcp_test.go
🧬 Code graph analysis (30)
backend/kubernetes/manager.go (1)
sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)
cli/cmd/kubectl-bind/cmd/kubectlBind.go (1)
cli/pkg/kubectl/bind/cmd/cmd.go (1)
New(57-109)
backend/client/client.go (1)
backend/auth/middleware.go (1)
ClientType(37-37)
cli/pkg/kubectl/bind-collections/cmd/cmd.go (1)
cli/pkg/kubectl/bind-collections/plugin/collections.go (1)
NewCollectionsOptions(44-50)
test/e2e/bind/happy-case_test.go (5)
test/e2e/framework/env.go (1)
WorkDir(26-26)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (2)
BindingResourceResponse(37-58)APIServiceExportTemplateRef(92-98)test/e2e/framework/browser.go (1)
SimulateBrowser(41-48)test/e2e/framework/bind.go (2)
Login(42-68)GetKubeBindRestClient(31-40)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)
cli/pkg/kubectl/bind-apiservice/plugin/konnector.go (2)
cli/pkg/kubectl/base/options.go (1)
Options(32-59)pkg/bootstrap/bootstrap.go (1)
Bootstrap(74-93)
contrib/kcp/test/e2e/binding.go (4)
test/e2e/framework/bind.go (1)
GetKubeBindRestClient(31-40)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
APIServiceExportTemplateRef(92-98)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)test/e2e/framework/clients.go (1)
ApiextensionsClient(45-49)
cli/pkg/kubectl/base/printer.go (2)
sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)sdk/apis/kubebind/v1alpha2/collection_types.go (1)
CollectionList(89-94)
cli/pkg/kubectl/bind/cmd/cmd.go (2)
cli/pkg/kubectl/bind-apiservice/cmd/cmd.go (1)
New(46-80)cli/pkg/kubectl/bind/plugin/bind.go (1)
NewBindOptions(88-100)
cli/pkg/kubectl/base/client.go (2)
cli/pkg/client/client.go (2)
Client(32-42)NewClient(73-97)cli/pkg/kubectl/config/config.go (1)
LoadConfigFromFile(67-90)
sdk/apis/kubebind/v1alpha2/register.go (1)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)
contrib/kcp/test/e2e/backend.go (1)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
InformerScope(59-59)
cli/pkg/kubectl/bind-collections/plugin/collections.go (3)
cli/pkg/kubectl/bind-login/plugin/login.go (1)
NewLoginOptions(83-92)sdk/apis/kubebind/v1alpha2/collection_types.go (1)
CollectionList(89-94)cli/pkg/kubectl/base/printer.go (1)
PrintObject(34-46)
cli/pkg/kubectl/bind/plugin/bind.go (6)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)cli/pkg/kubectl/base/options.go (1)
Options(32-59)cli/pkg/kubectl/config/config.go (3)
LoadConfigFromFile(67-90)BuildServerKey(274-280)ParseServerKey(283-288)cli/pkg/kubectl/bind-login/plugin/login.go (1)
NewLoginOptions(83-92)cli/pkg/kubectl/base/browser.go (1)
OpenBrowser(9-23)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)
backend/auth/handler.go (6)
backend/auth/types.go (3)
OIDCServiceProvider(59-67)AuthorizeRequest(44-50)AuthResponse(27-33)backend/auth/jwt.go (1)
JWTService(28-32)backend/client/client.go (2)
GetQueryParams(20-30)ClientParameters(8-17)backend/auth/middleware.go (2)
ClientType(37-37)ClientTypeCLI(41-41)backend/session/cookie.go (1)
MakeCookie(24-35)backend/session/session.go (2)
State(29-34)TokenInfo(36-39)
test/e2e/framework/bind.go (2)
cli/pkg/client/client.go (2)
Client(32-42)NewClient(73-97)cli/pkg/kubectl/bind-login/plugin/login.go (1)
NewLoginOptions(83-92)
cli/pkg/kubectl/bind-templates/cmd/cmd.go (1)
cli/pkg/kubectl/bind-templates/plugin/templates.go (1)
NewTemplatesOptions(50-56)
cli/pkg/client/client.go (4)
sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)sdk/apis/kubebind/v1alpha2/collection_types.go (1)
CollectionList(89-94)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)cli/pkg/kubectl/config/config.go (1)
LoadConfigFromFile(67-90)
sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (1)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (2)
APIServiceExportTemplateRef(92-98)BindingResourceResponse(37-58)
cli/pkg/kubectl/bind-login/cmd/cmd.go (2)
cli/pkg/kubectl/bind-login/plugin/login.go (1)
NewLoginOptions(83-92)cli/pkg/help/help.go (1)
Doc(33-37)
cli/pkg/kubectl/bind-templates/plugin/templates.go (2)
sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)cli/pkg/kubectl/base/printer.go (1)
PrintObject(34-46)
cli/pkg/kubectl/bind-apiservice/plugin/binder.go (4)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)sdk/apis/kubebind/v1alpha1/bindingprovider_types.go (1)
OAuth2CodeGrant(73-81)cli/pkg/kubectl/base/kubeconfig.go (3)
ParseRemoteKubeconfig(32-46)FindRemoteKubeconfig(48-79)EnsureKubeconfigSecret(86-139)cli/pkg/kubectl/bind-apiservice/plugin/bind.go (1)
BindAPIServiceOptions(50-72)
cli/pkg/kubectl/bind-apiservice/plugin/bind.go (6)
cli/pkg/kubectl/base/options.go (1)
Options(32-59)cli/pkg/kubectl/config/config.go (1)
LoadConfigFromFile(67-90)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)cli/pkg/kubectl/bind-login/plugin/login.go (1)
NewLoginOptions(83-92)cli/pkg/kubectl/base/kubeconfig.go (3)
ParseRemoteKubeconfig(32-46)FindRemoteKubeconfig(48-79)EnsureKubeconfigSecret(86-139)cli/pkg/client/client.go (1)
Client(32-42)
cli/pkg/kubectl/base/options.go (1)
cli/pkg/kubectl/config/config.go (1)
LoadConfigFromFile(67-90)
backend/http/handler.go (12)
backend/auth/types.go (1)
OIDCServiceProvider(59-67)backend/auth/handler.go (2)
AuthHandler(37-42)NewAuthHandler(44-51)backend/auth/middleware.go (4)
AuthMiddleware(50-54)NewAuthMiddleware(56-62)RequireAuth(127-145)GetAuthContext(120-125)backend/kubernetes/manager.go (1)
Manager(39-49)backend/auth/jwt.go (1)
NewJWTService(43-54)backend/spaserver/spaserver.go (2)
NewSPAReverseProxyServer(103-112)NewSPAFileServer(33-38)web/embed.go (1)
GetFileSystem(31-38)backend/client/client.go (1)
GetQueryParams(20-30)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)sdk/apis/kubebind/v1alpha2/collection_types.go (1)
CollectionList(89-94)sdk/apis/kubebind/v1alpha2/claimable_apis.go (1)
ClaimableAPIs(38-81)
cli/pkg/kubectl/bind-login/plugin/login.go (4)
cli/pkg/client/client.go (1)
Client(32-42)cli/pkg/kubectl/config/config.go (2)
LoadConfigFromFile(67-90)BuildServerKey(274-280)cli/pkg/kubectl/base/browser.go (1)
OpenBrowser(9-23)sdk/apis/kubebind/v1alpha1/bindingprovider_types.go (2)
BindingProvider(27-52)OAuth2CodeGrant(73-81)
backend/auth/types.go (2)
backend/auth/middleware.go (1)
ClientType(37-37)backend/session/session.go (1)
State(29-34)
backend/auth/middleware.go (2)
backend/session/session.go (2)
State(29-34)TokenInfo(36-39)backend/auth/jwt.go (1)
JWTService(28-32)
backend/server.go (2)
backend/auth/types.go (2)
OIDCServiceProvider(59-67)NewOIDCServiceProvider(69-83)backend/options/options.go (1)
Options(33-40)
contrib/kcp/test/e2e/kcp_test.go (7)
test/e2e/framework/kubeconfig.go (1)
ClientConfig(30-38)test/e2e/framework/kcp.go (3)
RandomString(73-78)NewWorkspace(101-162)WithStaticName(66-71)contrib/kcp/deploy/bootstrap.go (1)
Examples(40-40)test/e2e/framework/apply.go (1)
ApplyManifest(72-126)test/e2e/framework/env.go (1)
WorkDir(26-26)test/e2e/framework/browser.go (1)
SimulateBrowser(41-48)test/e2e/framework/bind.go (1)
Login(42-68)
🪛 ast-grep (0.39.7)
cli/pkg/client/client.go
[warning] 80-82: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
cli/pkg/kubectl/bind-login/plugin/login.go
[warning] 121-123: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true, //nolint:gosec
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: go-test
web/embed.go
[failure] 27-27:
pattern dist/*: no matching files found
[failure] 27-27:
pattern dist/*: no matching files found
[failure] 27-27:
pattern dist/*: no matching files found
[failure] 27-27:
pattern dist/*: no matching files found
🪛 golangci-lint (2.5.0)
backend/http/handler.go
[error] 40-40: could not import github.com/kube-bind/kube-bind/web (web/embed.go:27:12: pattern dist/*: no matching files found)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: goreleaser
- GitHub Check: go-test-e2e
🔇 Additional comments (16)
docs/content/setup/.pages (1)
1-6: Documentation navigation reordered.The navigation entries have been reordered to prioritize the kubectl plugin documentation earlier in the setup flow. This change improves the logical organization of the documentation.
docs/content/setup/kcp-setup.md (2)
92-92: Flag implementation verified.The
--consumer-scopeflag is properly implemented in the backend code atbackend/options/options.goline 142 as a string variable. The flag accepts "cluster" (for kcp workspaces) and "namespaced" (for Kubernetes) values, matching the documented usage indocs/content/setup/kcp-setup.mdline 92. The implementation aligns with test code usage across the codebase.
135-139: File paths verified and confirmed correct.The referenced YAML files exist at the documented paths in
contrib/kcp/deploy/examples/with the expected resource types:
template-cowboys.yamlandtemplate-sheriffs.yamlcontainAPIServiceExportTemplateresourcescollection-wildwest.yamlcontains aCollectionresourceThe documentation paths and file references are accurate. The progression of creating templates before a collection follows a logical order appropriate for the resource types involved.
cli/pkg/kubectl/bind/cmd/cmd.go (3)
36-54: LGTM! Clear and comprehensive examples.The updated examples effectively demonstrate both UI and CLI workflows, covering common use cases for binding templates and opening the kube-bind UI.
60-70: LGTM! Command metadata is clear and well-documented.The updated
Use,Short, andLongfields accurately describe the dual-mode functionality (UI and CLI), providing users with clear guidance on how to use the command.
73-86: LGTM! Argument validation is sound.The validation logic correctly enforces the 0-or-1 argument constraint and ensures URL arguments have the proper scheme prefix. Error messages are clear and actionable.
cli/pkg/kubectl/bind/plugin/authenticate.go (1)
39-42: Update the comment to reference the v0.6.0 API changes.The comment on line 39 is outdated and should clarify why the 0.6.0 minimum is required. Version v0.6.0 introduces significant API changes including new Catalog API CRDs (Collection, APIServiceExportTemplate), enhanced permission claims with NamedResources, and provider-side namespace management. Update the comment as follows:
- // Check if provider is higher than 0.4.8, we need to have same version of kube-bind to use this provider. + // Providers must be v0.6.0 or later to support the new Catalog API CRDs, enhanced Permission Claims with NamedResources, and provider-side namespace management.The documentation reference at
docs/content/contributing/guides/publishing-a-new-kube-bind-release.md:85(PREV_TAG=v0.4.8) is historical context for the v0.5.0 release notes and does not require updating.sdk/apis/kubebind/v1alpha2/register.go (1)
62-62: LGTM!The type registration correctly reflects the rename from
BindingResponsetoBindingResourceResponse, aligning with the API refactoring changes in this PR.web/embed.go (2)
30-38: LGTM!The panic on
fs.Subfailure is appropriate for this initialization function. If the embedded filesystem doesn't contain the expecteddistdirectory, it indicates a critical build configuration error that should fail fast at startup rather than at runtime.
27-28: Build orchestration is correctly implemented; original concern is incorrect.The codebase has proper build sequencing in place:
- CI Workflow (.github/workflows/ci.yaml): The
go-test-e2ejob explicitly runsmake build-web(which executesnpm run build) beforemake test-e2e, ensuring thedist/directory is populated before Go tests execute.- Docker Build: The Dockerfile uses multi-stage builds with the Node stage (
ui-build-env) executingnpm run buildbefore the Go stage copies the compileddist/directory.- Repository State: The
web/dist/directory exists and is tracked in the repository (verified viagit status).- Makefile: Provides explicit
build-webtarget for consistent frontend compilation.The static analysis warning about unmatched files is a development-time concern that occurs before the frontend is built locally, but does not prevent CI/CD or Docker builds from succeeding.
web/.eslintrc.cjs (1)
14-16: No changes required—component naming is safe.The verification identified four single-word component names (App, BindingResult, Resources, TemplateBindingModal). None conflict with native HTML elements or reserved custom element names. The
vue/multi-word-component-namesrule disable is acceptable for this project's naming conventions.cli/pkg/kubectl/bind-login/cmd/cmd.go (1)
43-79: Command wiring looks solid.The RunE flow cleanly composes logging, Complete/Validate, and Run on the shared options, so this command will integrate consistently with the other plugin verbs. Nicely done.
contrib/kcp/test/e2e/binding.go (1)
19-68: End-to-end helper reads well.The helper stitches together BindableResources, binder execution, and CRD polling cleanly, which should keep the happy-path e2e concise.
web/src/components/TemplateBindingModal.vue (1)
1-198: Modal UX and validation look great.Thanks for enforcing RFC1123-style names, auto-suggesting defaults, and rendering the template details in one place; this should make the binding flow far clearer for users.
backend/auth/handler.go (2)
171-211: Validateredirect_urlbefore redirecting.
authCode.RedirectURLis taken straight from the inbound request and later used (with access_token appended) as the redirect target. There is no allowlist or same-origin check, so an attacker can point it at an arbitrary domain and receive the JWT/session data. Please validate the redirect against a strict allowlist (or require a relative path) before callingurl.Parse/http.Redirect.
199-201: Mark the session cookie as Secure.
secure := falsecauses the auth session cookie to be sent over plain HTTP if any endpoint is reachable that way, exposing credentials. Derive the value from the request (e.g.,r.TLS != nilor trusted proto headers) so it is always Secure under HTTPS.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/kcp/test/e2e/kcp_test.go (1)
21-22: Switch tofilepath.Joinfor filesystem paths.
path.Joinis URL-focused and leaves Windows drive letters mixed with forward slashes. Usingfilepath.Joinkeeps the generated kubeconfig location valid across OSes that might run these tests. Please also swap the import accordingly.- "path" + "path/filepath"- kubeBindConfig := path.Join(framework.WorkDir, "kube-bind-config-kcp.yaml") + kubeBindConfig := filepath.Join(framework.WorkDir, "kube-bind-config-kcp.yaml")Also applies to: 168-176
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/kcp/test/e2e/kcp_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
Repo: kube-bind/kube-bind PR: 333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
📚 Learning: 2025-09-19T06:28:44.853Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go:148-150
Timestamp: 2025-09-19T06:28:44.853Z
Learning: In kube-bind, permission claims must all have the same scope (either all cluster-scoped or all namespace-scoped). Mixed scopes are not allowed, which means the isClusterScoped flag can be safely determined from any processed schema in the export.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
🧬 Code graph analysis (1)
contrib/kcp/test/e2e/kcp_test.go (7)
test/e2e/framework/kubeconfig.go (1)
ClientConfig(30-38)test/e2e/framework/kcp.go (3)
RandomString(73-78)NewWorkspace(101-162)WithStaticName(66-71)contrib/kcp/deploy/bootstrap.go (1)
Examples(40-40)test/e2e/framework/apply.go (1)
ApplyManifest(72-126)test/e2e/framework/env.go (1)
WorkDir(26-26)test/e2e/framework/browser.go (1)
SimulateBrowser(41-48)test/e2e/framework/bind.go (1)
Login(42-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: go-test
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: goreleaser
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/http/handler.go (1)
199-210: Derive request scheme from TLS context.Line 203 hardcodes
http://when constructing the authorize URL, which will fail for HTTPS deployments or those behind TLS-terminating proxies. The scheme should be derived fromr.TLSor theX-Forwarded-Protoheader.Apply this diff:
func (h *handler) handleServiceExport(w http.ResponseWriter, r *http.Request) { logger := klog.FromContext(r.Context()).WithValues("method", r.Method, "url", r.URL.String()) params := client.GetQueryParams(r) - oidcAuthorizeURL := params.WithParams(fmt.Sprintf("http://%s/api/authorize", r.Host)) + scheme := "http" + if proto := r.Header.Get("X-Forwarded-Proto"); proto != "" { + scheme = strings.Split(proto, ",")[0] + } else if r.TLS != nil { + scheme = "https" + } + oidcAuthorizeURL := params.WithParams(fmt.Sprintf("%s://%s/api/authorize", scheme, r.Host))
🧹 Nitpick comments (3)
cli/pkg/kubectl/bind-templates/plugin/templates.go (3)
83-86: Add validation logic or document why validation is not required.The
Validatemethod returnsnilwithout performing any validation. If no validation is needed at this stage, add a comment explaining this. Otherwise, consider adding appropriate validation (e.g., checking if required options are set).
107-116: Consider simplifying the output format check.The condition on line 109 can be simplified. The print flags typically handle empty string checks internally.
Consider this refactor:
- if o.Print.OutputFormat != nil && *o.Print.OutputFormat != "" { + if o.Print.OutputFormat != nil && *o.Print.OutputFormat != "" && *o.Print.OutputFormat != "wide" { templates.SetGroupVersionKind(kubebindv1alpha2.SchemeGroupVersion.WithKind("APIServiceExportTemplateList")) return o.printer.PrintObj(templates, o.IOStreams.Out) }Or check with the print flags to confirm that empty string always means table output is expected.
118-145: Consider handling empty templates list and potential write errors.The table display implementation could be improved:
- Consider displaying a user-friendly message when the templates list is empty
- While
w.Flush()error is properly returned, intermediatefmt.Fprintfcalls (lines 122, 141) have their errors silently ignored, which could mask write failuresConsider this enhancement:
func (o *TemplatesOptions) displayTemplatesTable(templates *kubebindv1alpha2.APIServiceExportTemplateList) error { + if len(templates.Items) == 0 { + fmt.Fprintln(o.Options.IOStreams.Out, "No templates found") + return nil + } + w := tabwriter.NewWriter(o.Options.IOStreams.Out, 0, 0, 3, ' ', 0) fmt.Fprintf(w, "NAME\tRESOURCES\tPERMISSIONCLAIMS\tAGE\n")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
backend/auth/handler.go(1 hunks)backend/auth/types.go(2 hunks)backend/client/client.go(1 hunks)backend/http/handler.go(8 hunks)backend/session/session.go(1 hunks)cli/go.mod(7 hunks)cli/pkg/client/client.go(1 hunks)cli/pkg/config/config.go(1 hunks)cli/pkg/kubectl/base/client.go(1 hunks)cli/pkg/kubectl/base/options.go(3 hunks)cli/pkg/kubectl/bind-apiservice/plugin/bind.go(5 hunks)cli/pkg/kubectl/bind-collections/plugin/collections.go(1 hunks)cli/pkg/kubectl/bind-login/plugin/login.go(1 hunks)cli/pkg/kubectl/bind-templates/plugin/templates.go(1 hunks)cli/pkg/kubectl/bind/plugin/bind.go(5 hunks)contrib/kcp/test/e2e/binding.go(1 hunks)contrib/kcp/test/e2e/kcp_test.go(3 hunks)test/e2e/framework/bind.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/client/client.go
- cli/pkg/kubectl/bind-login/plugin/login.go
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-09-19T05:56:35.969Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: backend/controllers/servicenamespace/servicenamespace_reconcile.go:81-98
Timestamp: 2025-09-19T05:56:35.969Z
Learning: In kube-bind, RBAC permissions for PermissionClaims use "*" verbs intentionally. This is a design decision based on: 1) permissions are scoped to consumer-owned provider namespaces, limiting blast radius, 2) bidirectional resource flow requires broad permissions for operations like initial resource creation from consumer side, 3) kube-bind's architecture prioritizes operational simplicity over granular RBAC within the namespace security boundary.
Applied to files:
backend/auth/types.gobackend/http/handler.go
📚 Learning: 2025-09-19T06:28:44.853Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go:148-150
Timestamp: 2025-09-19T06:28:44.853Z
Learning: In kube-bind, permission claims must all have the same scope (either all cluster-scoped or all namespace-scoped). Mixed scopes are not allowed, which means the isClusterScoped flag can be safely determined from any processed schema in the export.
Applied to files:
backend/auth/types.gocontrib/kcp/test/e2e/kcp_test.gobackend/http/handler.go
📚 Learning: 2025-10-22T06:41:37.475Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 342
File: docs/content/usage/migration.md:34-37
Timestamp: 2025-10-22T06:41:37.475Z
Learning: In the kube-bind repository documentation and examples, "mangodb" (with group "mangodb.com" and resource "mangodbs") is an intentional fictional example name chosen to avoid referencing real products like MongoDB. It should not be treated as a typo.
Applied to files:
backend/auth/types.go
📚 Learning: 2025-09-12T09:05:29.762Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.762Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
backend/auth/types.gobackend/http/handler.go
📚 Learning: 2025-09-22T13:20:49.952Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: sdk/apis/kubebind/v1alpha2/boundchema_types.go:49-0
Timestamp: 2025-09-22T13:20:49.952Z
Learning: In kube-bind, BoundSchema.ResourceGroupName() is only used for CRDs (Custom Resource Definitions), and CRDs must always have non-empty API groups. Therefore, handling empty groups (core API group "") is not necessary in this context, unlike general Kubernetes GroupResource handling.
Applied to files:
backend/auth/types.gobackend/http/handler.go
📚 Learning: 2025-09-12T08:55:41.860Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.860Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
backend/auth/types.gobackend/http/handler.go
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
Repo: kube-bind/kube-bind PR: 333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
📚 Learning: 2025-09-12T08:40:15.290Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.290Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
backend/http/handler.go
📚 Learning: 2025-09-22T13:32:29.499Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: pkg/konnector/controllers/cluster/claimedresources/claimedresources_controller.go:255-263
Timestamp: 2025-09-22T13:32:29.499Z
Learning: In kube-bind's claimedresources controller (pkg/konnector/controllers/cluster/claimedresources/claimedresources_controller.go), the controller does not run if apiServiceExport is not set. This means nil-checks for c.apiServiceExport are unnecessary since the controller lifecycle ensures it's always non-nil when active.
Applied to files:
backend/http/handler.gocli/pkg/kubectl/bind-apiservice/plugin/bind.go
📚 Learning: 2025-09-22T13:20:49.952Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: sdk/apis/kubebind/v1alpha2/boundchema_types.go:49-0
Timestamp: 2025-09-22T13:20:49.952Z
Learning: In kube-bind, there are two different ResourceGroupName() methods: BoundSchema.ResourceGroupName() for CRDs (always non-empty groups) uses simple fmt.Sprintf formatting, while APIServiceExportRequestResource.ResourceGroupName() for export requests handles empty groups by converting to "core". BoundSchema is exclusively for CRDs which cannot have empty API groups per Kubernetes validation.
Applied to files:
backend/http/handler.go
🧬 Code graph analysis (13)
backend/auth/types.go (2)
backend/auth/middleware.go (1)
ClientType(37-37)backend/session/session.go (1)
State(29-34)
cli/pkg/kubectl/bind-collections/plugin/collections.go (2)
cli/pkg/kubectl/base/options.go (2)
Options(34-59)NewOptions(62-67)sdk/apis/kubebind/v1alpha2/collection_types.go (1)
CollectionList(89-94)
cli/pkg/kubectl/bind-templates/plugin/templates.go (2)
cli/pkg/kubectl/base/options.go (2)
Options(34-59)NewOptions(62-67)sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)
backend/auth/handler.go (6)
backend/auth/types.go (3)
OIDCServiceProvider(61-69)AuthorizeRequest(46-52)AuthResponse(27-33)backend/auth/jwt.go (1)
JWTService(28-32)backend/client/client.go (2)
GetQueryParams(20-30)ClientParameters(8-17)backend/auth/middleware.go (2)
ClientType(37-37)ClientTypeCLI(41-41)backend/session/cookie.go (1)
MakeCookie(24-35)backend/session/session.go (2)
State(29-34)TokenInfo(36-39)
cli/pkg/kubectl/bind/plugin/bind.go (4)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)cli/pkg/kubectl/base/options.go (1)
Options(34-59)cli/pkg/kubectl/base/browser.go (1)
OpenBrowser(9-23)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)
contrib/kcp/test/e2e/binding.go (4)
test/e2e/framework/bind.go (1)
GetKubeBindRestClient(32-46)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
APIServiceExportTemplateRef(92-98)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)test/e2e/framework/clients.go (1)
ApiextensionsClient(45-49)
contrib/kcp/test/e2e/kcp_test.go (9)
test/e2e/framework/dex.go (1)
StartDex(53-93)test/e2e/framework/kubeconfig.go (1)
ClientConfig(30-38)test/e2e/framework/kcp.go (3)
RandomString(73-78)NewWorkspace(101-162)WithStaticName(66-71)test/e2e/framework/konnector.go (1)
StartKonnector(35-75)contrib/kcp/deploy/bootstrap.go (1)
Examples(40-40)test/e2e/framework/apply.go (1)
ApplyManifest(72-126)test/e2e/framework/env.go (1)
WorkDir(26-26)test/e2e/framework/browser.go (1)
SimulateBrowser(41-48)test/e2e/framework/bind.go (1)
Login(48-74)
backend/http/handler.go (10)
backend/auth/types.go (1)
OIDCServiceProvider(61-69)backend/auth/handler.go (2)
AuthHandler(37-42)NewAuthHandler(44-51)backend/auth/middleware.go (4)
AuthMiddleware(50-54)NewAuthMiddleware(56-62)RequireAuth(127-145)GetAuthContext(120-125)backend/kubernetes/manager.go (1)
Manager(39-49)backend/auth/jwt.go (1)
NewJWTService(43-54)backend/spaserver/spaserver.go (2)
NewSPAReverseProxyServer(103-112)NewSPAFileServer(33-38)web/embed.go (1)
GetFileSystem(31-38)backend/client/client.go (1)
GetQueryParams(20-30)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)sdk/apis/kubebind/v1alpha2/claimable_apis.go (1)
ClaimableAPIs(38-81)
cli/pkg/kubectl/base/options.go (1)
cli/pkg/config/config.go (5)
Server(40-47)Config(29-37)GetDefaultConfigFilePath(64-70)NewConfig(50-61)LoadConfigFromFile(78-101)
cli/pkg/kubectl/bind-apiservice/plugin/bind.go (5)
cli/pkg/kubectl/base/options.go (1)
Options(34-59)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)cli/pkg/kubectl/base/kubeconfig.go (3)
ParseRemoteKubeconfig(32-46)FindRemoteKubeconfig(48-79)EnsureKubeconfigSecret(86-139)cli/pkg/client/client.go (1)
Client(36-44)
cli/pkg/kubectl/base/client.go (2)
cli/pkg/kubectl/base/options.go (1)
Options(34-59)cli/pkg/client/client.go (3)
Client(36-44)NewClient(65-87)WithInsecure(58-62)
cli/pkg/client/client.go (4)
sdk/apis/kubebind/v1alpha2/apiserviceexporttemplate_types.go (1)
APIServiceExportTemplateList(98-103)sdk/apis/kubebind/v1alpha2/collection_types.go (1)
CollectionList(89-94)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)cli/pkg/config/config.go (2)
Server(40-47)Config(29-37)
test/e2e/framework/bind.go (3)
cli/pkg/client/client.go (3)
Client(36-44)NewClient(65-87)WithInsecure(58-62)cli/pkg/config/config.go (1)
LoadConfigFromFile(78-101)cli/pkg/kubectl/bind-login/plugin/login.go (1)
NewLoginOptions(77-85)
🪛 ast-grep (0.39.7)
cli/pkg/client/client.go
[warning] 79-81: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 golangci-lint (2.5.0)
backend/http/handler.go
[error] 40-40: could not import github.com/kube-bind/kube-bind/web (web/embed.go:27:12: pattern dist/*: no matching files found)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: goreleaser
- GitHub Check: lint
- GitHub Check: go-test
- GitHub Check: go-test-e2e
🔇 Additional comments (29)
cli/go.mod (1)
1-138: Clarification: Dependencies are in root go.mod, not cli/go.mod.The AI summary's claims about adding JWT and web dependencies are accurate but apply to the root
go.mod, not thecli/go.modfile under review. The rootgo.modcontains:
- JWT dependency:
github.com/golang-jwt/jwt/v5 v5.3.0- Web module replace directive:
github.com/kube-bind/kube-bind/web => ./web- Web module require:
github.com/kube-bind/kube-bind/web v0.0.0-...The
cli/go.moddoes not include these as direct requirements because it inherits them transitively through its direct requirement on the root module (github.com/kube-bind/kube-bind). The marked changes incli/go.modare version updates to indirect dependencies, consistent with ago mod tidyoperation.This structure is correct if the CLI package does not directly use JWT or web functionality.
cli/pkg/kubectl/bind-collections/plugin/collections.go (3)
37-60: LGTM!The struct definition and initialization follow standard kubectl plugin patterns. The use of empty default output appropriately triggers table format for the default case.
62-85: LGTM!The lifecycle methods properly handle argument processing, option completion, and validation with appropriate error handling.
87-102: LGTM!The execution flow is clear with proper error handling and appropriate error wrapping for debugging context.
cli/pkg/kubectl/bind-templates/plugin/templates.go (2)
53-67: LGTM!The constructor and flag binding logic follow standard kubectl plugin patterns and properly initialize all required fields.
88-104: Based on the verification, the code incli/pkg/kubectl/bind-templates/plugin/templates.golines 88-104 follows the standard Go error handling pattern correctly.If an error occurs, the error will be set with implementation of the error interface, otherwise it will be set to nil. The implementation of
GetTemplates()confirms this pattern: it returnsnilpointer only when an error is non-nil, and returns a valid pointer when error is nil.The
Runmethod's error checks with early returns provide adequate nil safety:
- If
GetAuthenticatedClient()fails, the method returns before usingclient- If
GetTemplates()fails, the method returns before callingdisplayTemplates(templates)This is the idiomatic Go approach and aligns with standard library practices.
Likely an incorrect or invalid review comment.
cli/pkg/kubectl/bind-apiservice/plugin/bind.go (8)
26-27: LGTM!The import additions support the new template-based binding flow and namespace management functionality.
Also applies to: 31-31, 39-39
100-112: LGTM!The printer initialization is correct and error handling is appropriate.
116-132: LGTM!The validation logic correctly handles the new template-based binding flow and maintains appropriate constraints for remote kubeconfig parameters.
138-171: LGTM!The refactored Run method provides clear separation of concerns between obtaining the binding response and processing it through the Binder. The error handling and flow are appropriate.
173-225: LGTM!The method correctly handles multiple kubeconfig sources with appropriate fallthrough behavior when explicit namespace/name flags are provided.
227-249: LGTM!The namespace creation logic is straightforward with appropriate error handling and user feedback.
313-325: LGTM!The function correctly constructs a binding request with a template reference and delegates to the client.
279-281: Code is correct; no changes required.Verification confirms that
OAuth2CodeGrantis the only supported authentication method in the binding flow. TheBindingResponseAuthenticationstruct contains only a single field—OAuth2CodeGrant—and theAuthenticationMethod.Methodenum is restricted to"OAuth2CodeGrant"only. The nil check at lines 279-281 appropriately handles this single supported method.contrib/kcp/test/e2e/binding.go (1)
19-69: LGTM!The test helper function is well-structured. It properly sequences the binding workflow: creating the REST client, sending the binding request, using the binder to process the response, and polling for CRD creation. The use of testify/require for assertions and wait.Eventually for polling is appropriate for test code.
backend/auth/types.go (1)
17-95: LGTM!The authentication types are well-defined with appropriate JSON/form bindings. The use of time.Time for ExpiresAt provides proper type safety for token expiration handling, and the struct definitions align with the authentication flows implemented in the handler and middleware.
test/e2e/framework/bind.go (2)
32-46: LGTM!The helper function correctly loads configuration, retrieves the current server, creates an authenticated client with insecure mode enabled, and provides proper error handling with descriptive assertions.
48-74: LGTM!The Login helper appropriately constructs CLI flags, parses them, and executes the login flow with proper context and error handling. The conditional flag construction for cluster and config file is correct.
backend/auth/handler.go (2)
53-92: LGTM!The HandleAuthorize method correctly handles both POST (JSON) and GET (query params) requests, validates required parameters, encodes the authorization request state, and redirects to the OIDC provider.
214-277: LGTM!The helper methods provide appropriate functionality: respondWithError differentiates between CLI (JSON) and UI (plain text) responses, generateCookieName namespaces cookies by cluster ID, createSessionState extracts token information from OIDC responses, and unwrapJWT correctly parses JWT structure.
backend/http/handler.go (5)
71-108: LGTM!The NewHandler constructor correctly initializes JWT service, auth handler, and auth middleware with appropriate error handling. The wiring of authentication components is clean and follows the expected initialization pattern.
110-145: LGTM!The AddRoutes method properly organizes routes into public API endpoints, authentication endpoints, and protected API routes with authentication middleware. The SPA serving logic correctly handles both development proxy mode and production static file serving, and returning an error instead of panicking on SPA server creation is the correct approach.
242-285: LGTM!Both handleTemplates and handleCollections correctly retrieve query parameters, list resources from the backend, and encode JSON responses with appropriate error handling.
287-374: LGTM!The handleBind method correctly retrieves authentication context from middleware, handles resources, enforces request body size limits (preventing DoS), parses and validates the bind request, retrieves the template, constructs the response, and returns JSON. The request body size limit and proper error handling for MaxBytesError address previous concerns.
376-421: LGTM!The helper functions (listTemplates, listCollections, handleBindableResources, prepareNoCache, getLogger) are straightforward and provide appropriate functionality with proper error handling.
cli/pkg/client/client.go (4)
89-127: LGTM!The GetTemplates and GetCollections methods correctly construct endpoint URLs, perform authenticated GET requests, and decode JSON responses with proper error handling and resource cleanup.
129-152: LGTM!The Bind method correctly marshals the request, performs an authenticated POST, and decodes the response with appropriate error wrapping and resource cleanup.
154-190: LGTM!The Get, Post, and Do methods correctly construct HTTP requests, add authentication headers, set appropriate content types, and delegate to the underlying HTTP client.
192-255: LGTM!The helper methods provide appropriate functionality: addAuthHeaders validates token presence and expiration with helpful error messages, isTokenValid checks token validity with a 5-minute buffer, buildEndpointURL constructs proper API URLs with cluster context, and getAuthorizationHeader formats the authorization header correctly.
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
0a414a1 to
fa967cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/setup/quickstart.md (1)
42-49: Update dex configuration callback URL to match backend changes.The dex config example at line 46 specifies
http://127.0.0.1:8080/callback, but the backend is configured at line 89 withhttp://127.0.0.1:8080/api/callback. These must match for OIDC authentication to function.Apply this diff to align the dex configuration with the backend callback URL:
staticClients: - id: kube-bind redirectURIs: - - 'http://127.0.0.1:8080/callback' + - 'http://127.0.0.1:8080/api/callback' name: 'Kube Bind' secret: ZXhhbXBsZS1hcHAtc2VjcmV0
♻️ Duplicate comments (5)
cli/pkg/kubectl/base/options.go (1)
128-133: Fix parsing of--servervalues containing@.This issue was previously flagged but remains unresolved. The stored config format is
cluster@serverURL, but this code treats the input asserver@cluster, causing incorrect lookups when users provide the stored format. The previous review suggested detecting which part is the URL to handle both orders correctly.Apply the fix suggested in the previous review:
- case o.Server != "" && strings.Contains(o.Server, "@"): - // Server specified in server@cluster format - parts := strings.SplitN(o.Server, "@", 2) - o.Server = parts[0] - o.Cluster = parts[1] + case o.Server != "" && strings.Contains(o.Server, "@"): + parts := strings.SplitN(o.Server, "@", 2) + // Config keys are stored as cluster@server. If the second segment looks + // like a URL, treat the input as that stored format; otherwise fall back + // to server@cluster for backward compatibility. + if strings.Contains(parts[1], "://") { + o.Cluster = parts[0] + o.Server = parts[1] + } else { + o.Server = parts[0] + o.Cluster = parts[1] + }cli/pkg/kubectl/bind-login/plugin/login.go (1)
245-247: Preserve base path when appending /api/exports (previously flagged).This issue was identified in a previous review but remains unaddressed. Overwriting
url.Pathwith/api/exportsdrops any existing prefix (e.g.,/backend), breaking login for installations served from a subpath.Apply this diff to preserve the original base path:
+import "path" + func (o *LoginOptions) getProvider(ctx context.Context) (*kubebindv1alpha2.BindingProvider, error) { url, err := url.Parse(o.Options.Server) // ... existing code ... if !strings.Contains(url.Path, "/api/exports") { - url.Path = "/api/exports" + basePath := strings.TrimSuffix(url.Path, "/") + url.Path = path.Join(basePath, "/api/exports") }cli/pkg/kubectl/bind/plugin/bind.go (1)
282-288: Fix callback listener security issue and race condition.This issue was previously flagged but remains unresolved. After selecting a loopback port with
net.Listen("127.0.0.1:0")at Line 282, the code closes that listener at Line 288 and restarts the server at Line 356 withAddr: fmt.Sprintf(":%d", port), which rebinds on all network interfaces. This exposes the callback endpoint to the network and introduces a race where another process can steal the port between Lines 288 and 361.Keep the original loopback listener open and serve it directly with
server.Serve(listener)to maintain local-only binding and eliminate the race condition.Apply this diff:
listener, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { return nil, 0, fmt.Errorf("failed to find available port: %w", err) } port := listener.Addr().(*net.TCPAddr).Port - listener.Close() // Setup HTTP handler mux := http.NewServeMux() mux.HandleFunc("/callback", func(w http.ResponseWriter, r *http.Request) { @@ }) server := &http.Server{ ReadTimeout: time.Minute * 5, - Addr: fmt.Sprintf(":%d", port), Handler: mux, } go func() { - if err := server.ListenAndServe(); err != http.ErrServerClosed { + if err := server.Serve(listener); err != nil && err != http.ErrServerClosed { select { case errCh <- err: default: } } }()Also applies to: 354-367
cli/pkg/kubectl/bind-apiservice/plugin/bind.go (2)
64-64: DryRun flag is not wired to the implementation.The
DryRunfield is exposed via the CLI flag, but it is never passed toBinderOptionsduring binder initialization (lines 158-167). TheBinderOptionsstruct includes aDryRunfield, but it remains unset. Users who specify--dry-runwill see no effect.Apply this diff to wire the flag:
binderOpts := &BinderOptions{ IOStreams: b.Options.IOStreams, SkipKonnector: b.SkipKonnector, KonnectorImageOverride: b.KonnectorImageOverride, DowngradeKonnector: b.DowngradeKonnector, RemoteKubeconfigFile: b.remoteKubeconfigFile, RemoteKubeconfigNamespace: b.remoteKubeconfigNamespace, RemoteKubeconfigName: b.remoteKubeconfigName, RemoteNamespace: b.remoteNamespace, + DryRun: b.DryRun, }Additionally, ensure the
Binderimplementation respects theDryRunoption by checking it before performing mutations and logging planned actions instead.Also applies to: 93-93, 158-167
87-87: Fix the default value reference in the flag help text.The flag is bound to
b.remoteKubeconfigNamebut incorrectly referencesb.remoteKubeconfigNamespaceas the default value.Apply this diff:
- cmd.Flags().StringVar(&b.remoteKubeconfigName, "remote-kubeconfig-name", b.remoteKubeconfigNamespace, "The name of the remote kubeconfig secret to read from") + cmd.Flags().StringVar(&b.remoteKubeconfigName, "remote-kubeconfig-name", b.remoteKubeconfigName, "The name of the remote kubeconfig secret to read from")
🧹 Nitpick comments (5)
docs/content/setup/kcp-setup.md (1)
92-92: Document the purpose of--consumer-scope=cluster.The new
--consumer-scope=clusterflag is added to the backend startup command but lacks explanation. Clarify what this flag controls and why it is necessary for this setup.cli/pkg/kubectl/base/options.go (2)
57-57: Consider clarifying the comment.The comment states "Server is the loaded kube-bind server" but the field is named
server(lowercase). Consider updating to "server is the loaded kube-bind server" for consistency.
200-202: Consider documenting preconditions.The
GetConfig()method returnso.config, which is only populated afterComplete()executes successfully. Consider adding a comment noting that this method should only be called afterComplete()to avoid returning nil.cli/pkg/kubectl/bind-login/plugin/login.go (1)
338-357: Consider extracting HTML template.The inline HTML string is verbose and reduces readability. Consider extracting it to a constant or using a template.
Example refactor:
const callbackHTMLTemplate = `<!DOCTYPE html> <html> <head> <title>Kube-Bind Authentication</title> <style> body { font-family: Arial, sans-serif; text-align: center; margin-top: 100px; } .success { color: green; } .error { color: red; } </style> </head> <body> <h1>Kube-Bind Authentication</h1> <div class="%s"> <h2>%s</h2> <p>You can now close this window and return to the CLI.</p> </div> </body> </html>` // In handler: w.Header().Set("Content-Type", "text/html") statusClass := "success" message := "Authentication Successful!" if token.Error != "" { statusClass = "error" message = "Authentication Failed" } fmt.Fprintf(w, callbackHTMLTemplate, statusClass, message)cli/pkg/kubectl/bind/plugin/bind.go (1)
142-145: Consider removing redundant client config retrieval.The client config retrieved at Line 142 is not used until Line 229, where it is retrieved again. Since this early retrieval serves no immediate purpose, consider removing Lines 142-145 to simplify the code.
Apply this diff:
func (b *BindOptions) runWithCallback(ctx context.Context, _ chan<- string) error { - _, err := b.Options.ClientConfig.ClientConfig() - if err != nil { - return fmt.Errorf("failed to get client config: %w", err) - } - // Generate session ID. It is used to verify callback. - sessionID, err := generateRandomString(32) + sessionID, err := generateRandomString(32) if err != nil { return fmt.Errorf("failed to generate session ID: %w", err) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
cli/go.sumis excluded by!**/*.sumcontrib/kcp/go.sumis excluded by!**/*.sumdocs/generators/cli-doc/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumweb/dist/assets/index.33c2e7b8.cssis excluded by!**/dist/**web/dist/assets/index.4caa6f6c.jsis excluded by!**/dist/**web/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (11)
.gitignore(1 hunks)cli/go.mod(1 hunks)cli/pkg/kubectl/base/options.go(3 hunks)cli/pkg/kubectl/bind-apiservice/plugin/bind.go(5 hunks)cli/pkg/kubectl/bind-login/plugin/login.go(1 hunks)cli/pkg/kubectl/bind/plugin/bind.go(3 hunks)contrib/kcp/go.mod(6 hunks)docs/content/setup/kcp-setup.md(3 hunks)docs/content/setup/quickstart.md(2 hunks)docs/generators/cli-doc/go.mod(0 hunks)go.mod(3 hunks)
💤 Files with no reviewable changes (1)
- docs/generators/cli-doc/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/go.mod
- .gitignore
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-12T08:40:15.290Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.290Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
docs/content/setup/quickstart.md
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
Repo: kube-bind/kube-bind PR: 333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
docs/content/setup/kcp-setup.md
📚 Learning: 2025-09-22T13:32:29.499Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 304
File: pkg/konnector/controllers/cluster/claimedresources/claimedresources_controller.go:255-263
Timestamp: 2025-09-22T13:32:29.499Z
Learning: In kube-bind's claimedresources controller (pkg/konnector/controllers/cluster/claimedresources/claimedresources_controller.go), the controller does not run if apiServiceExport is not set. This means nil-checks for c.apiServiceExport are unnecessary since the controller lifecycle ensures it's always non-nil when active.
Applied to files:
cli/pkg/kubectl/bind-apiservice/plugin/bind.go
🧬 Code graph analysis (4)
cli/pkg/kubectl/base/options.go (1)
cli/pkg/config/config.go (5)
Server(40-47)Config(29-37)GetDefaultConfigFilePath(64-70)NewConfig(50-61)LoadConfigFromFile(78-101)
cli/pkg/kubectl/bind/plugin/bind.go (4)
sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
BindingResourceResponse(37-58)cli/pkg/kubectl/base/options.go (1)
Options(34-59)cli/pkg/kubectl/base/browser.go (1)
OpenBrowser(9-23)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)
cli/pkg/kubectl/bind-login/plugin/login.go (5)
cli/pkg/kubectl/base/options.go (2)
Options(34-59)NewOptions(62-67)cli/pkg/config/config.go (1)
Server(40-47)backend/auth/types.go (1)
TokenResponse(35-44)cli/pkg/kubectl/base/browser.go (1)
OpenBrowser(9-23)sdk/apis/kubebind/v1alpha1/bindingprovider_types.go (2)
BindingProvider(27-52)OAuth2CodeGrant(73-81)
cli/pkg/kubectl/bind-apiservice/plugin/bind.go (3)
cli/pkg/kubectl/base/options.go (1)
Options(34-59)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)cli/pkg/kubectl/base/kubeconfig.go (3)
ParseRemoteKubeconfig(32-46)FindRemoteKubeconfig(48-79)EnsureKubeconfigSecret(86-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: verify
- GitHub Check: goreleaser
- GitHub Check: lint
- GitHub Check: go-test
- GitHub Check: go-test-e2e
🔇 Additional comments (13)
contrib/kcp/go.mod (3)
23-23: Verify the necessity of direct require on cli module.The
climodule is added to the require block with a pseudo-versionv0.0.0-20250515145715-d9f20e7c840d, but a replace directive already exists at line 7 that overrides this version (github.com/kube-bind/kube-bind/cli => ../../cli). Please confirm that this direct require is necessary or if it should be removed from the require block, allowing the replace directive alone to manage the dependency.
9-9: Web module integration is appropriately configured.The new replace directive for the web module aligns with the PR's API-first backend and dedicated frontend objective. The pseudo-version
v0.0.0-00010101000000-000000000000is the standard marker for local module replacements and is correct.
66-66: JWT dependency addition is appropriate.The inclusion of
github.com/golang-jwt/jwt/v5 v5.3.0as an indirect dependency aligns with the authentication flow refactoring to support OIDC + JWT mentioned in the PR summary.go.mod (4)
27-27: Confirm web module is directly imported in root backend code.The PR adds
github.com/kube-bind/kube-bind/webas a direct require with replace directivegithub.com/kube-bind/kube-bind/web => ./web. Please verify that the root backend package directly imports and uses types, functions, or assets from the web module. If the web module is only consumed transitively (e.g., only by CLI or other packages), it should be moved to an indirect dependency to keep the require block clean.
9-9: Web module integration configuration is correct.The replace directive for the web module points to the local
./webdirectory, which aligns with the monorepo structure and enables asset embedding into the Go binary as described in the PR objectives.
16-16: JWT dependency addition aligns with authentication refactoring.The addition of
github.com/golang-jwt/jwt/v5 v5.3.0supports the PR's backend authentication rework for OIDC + JWT flows serving both UI (cookie-based) and CLI (token-based) authentication paths.
56-57: Dependency version updates are consistent across modules.The updates to
github.com/PuerkitoBio/goquery(v1.10.3) andgithub.com/andybalholm/cascadia(v1.3.3) are aligned between the rootgo.modandcontrib/kcp/go.mod, maintaining consistency across the dependency graph.docs/content/setup/kcp-setup.md (2)
137-139: All referenced example files are present in the repository.The verification confirms that all three example files referenced in the documentation exist and are properly checked in:
contrib/kcp/deploy/examples/template-cowboys.yamlcontrib/kcp/deploy/examples/template-sheriffs.yamlcontrib/kcp/deploy/examples/collection-wildwest.yamlNo further action is required.
165-169: Update stale comment and clarify binding workflow and placeholder value.The comment on line 165 describes the old flow but the code implements a login-based workflow. Additionally, the
--skip-konnectorflag on line 169 is used without explanation, and the<logical-cluster-id>placeholder lacks clarification regarding whether to use the NAME column value (e.g., "cluster"), the full URL from the URL column, or the cluster ID extracted from the URL path (e.g., "2xh2v3gzjhn4tmve").Update the comment and add clarification:
-Generate the APIServiceExport YAML: +Perform login-based binding:Add inline documentation explaining that
--skip-konnectordefers konnector startup (see section "3. Start Konnector" below), and specify that<logical-cluster-id>should be replaced with the cluster ID from the URL column (the alphanumeric path segment following/clusters/).docs/content/setup/quickstart.md (1)
118-121: Verify endpoint change accuracy and impact on existing users.Unable to locate the kubectl-bind CLI implementation in the codebase to verify whether the command at line 121 is correct. While the enriched summary indicates this reflects a "shift to a more general API surface," the documentation change from
http://127.0.0.1:8080/exportstohttp://127.0.0.1:8080removes a path segment without explanation.Manually verify:
- Whether the current CLI implementation accepts the base URL without
/exportspath segment- Whether this is a breaking change requiring user migration documentation
- Whether a note should explain the endpoint structure change to users upgrading from previous versions
cli/pkg/kubectl/base/options.go (2)
89-93: LGTM!The new flag bindings are clear and correctly wired to the struct fields.
108-123: LGTM!The config file initialization logic correctly creates the directory structure with appropriate permissions (0700) and initializes an empty config file when needed.
cli/pkg/kubectl/bind-login/plugin/login.go (1)
383-389: LGTM!Proper use of
crypto/randfor security-sensitive random generation. The implementation is correct.
| sessionID, err := generateRandomString(32) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate session ID: %w", err) | ||
| } |
There was a problem hiding this comment.
Verify session ID in the callback handler.
The session ID is generated but never validated when the callback is received. This allows an attacker to craft a callback URL without a matching session ID, potentially enabling CSRF attacks against the local callback server.
Consider adding session ID verification in the callback handler. Store the expected session ID and validate it matches the session_id query parameter in the callback:
func (o *LoginOptions) startCallbackServerWithRandomPort(tokenCh chan<- *TokenResponse, errCh chan<- error) (*http.Server, string, error) {
+ expectedSessionID := "" // Store the session ID
// ... existing code ...
mux.HandleFunc("/callback", func(w http.ResponseWriter, r *http.Request) {
+ receivedSessionID := r.URL.Query().Get("session_id")
+ if receivedSessionID != expectedSessionID {
+ http.Error(w, "Invalid session", http.StatusBadRequest)
+ return
+ }
// ... rest of handler ...Alternatively, pass the session ID as a parameter to startCallbackServerWithRandomPort.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/pkg/kubectl/bind-login/plugin/login.go around lines 128 to 131, the
generated sessionID is not validated on OAuth callback which permits forged
callbacks; persist the expected sessionID and validate the incoming callback's
session_id query parameter against it (or pass the sessionID into
startCallbackServerWithRandomPort and have the server compare the request
parameter to the expected value), and on mismatch return an error/HTTP 400 and
reject the flow; ensure any storage used is thread-safe and the validation
happens before exchanging the code for tokens.
There was a problem hiding this comment.
| sessionID, err := generateRandomString(32) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate session ID: %w", err) | |
| } | |
| sessionID := rand.Text() |
rand.Text should be enough for things like this.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for rand.Text usage in the codebase
rg -n 'rand\.Text\(\)' --type=go
# Also search for rand package imports to understand usage patterns
rg -n 'k8s\.io/.*rand' --type=go -A 1
# Search for the util/rand package specifically
rg -n '"k8s.io/apimachinery/pkg/util/rand"' --type=go
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check the imports in the file that uses rand.Text()
head -50 test/e2e/framework/dex.go | rg 'import' -A 20
# Also check if there are other usages of rand.Text in the broader codebase
rg 'rand\.Text' --type=go -B 2 -A 2
# Search for the specific import of the rand package
rg '"k8s.io/apimachinery/pkg/util/rand"' -B 5 -A 5
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Get more context around the rand.Text usage to see what package it's from
cat test/e2e/framework/dex.go | head -30
# Search for any custom Text() method definitions on rand
ast-grep --pattern 'func $_(rand $_) Text() $_ { $$$ }'
# Check if there's a util/rand import anywhere that might be aliased
rg 'util/rand' --type=go
# Look for the full function signature and more context
rg 'func.*Text\(\)' --type=go -A 3
Length of output: 713
🏁 Script executed:
#!/bin/bash
# Get the full dex.go file around the rand.Text usage
sed -n '95,115p' test/e2e/framework/dex.go
# Check if there's a local rand package or helper in the framework
fd -e go . test/e2e/framework/ --exec grep -l 'func Text'
# Look for any rand helper or wrapper in the test framework
rg 'package.*rand' --type=go test/e2e/
# Check the exact line with more context
sed -n '100,110p' test/e2e/framework/dex.go
Length of output: 1043
🏁 Script executed:
#!/bin/bash
# Search for the Text function definition in the framework or test directories
rg 'func Text\(\)' --type=go
# Check if there's a rand.go file in the test framework
fd 'rand.go' test/
# Look for any file that defines a Text function that returns a string
ast-grep --pattern 'func Text() string { $$$ }'
# Check for any custom wrapper around crypto/rand
rg 'crypto/rand' test/e2e/framework/ -A 10 | head -50
Length of output: 927
🏁 Script executed:
#!/bin/bash
# Look for all go files in test/e2e/framework that might define Text
rg 'func.*Text' test/e2e/framework/ --type=go
# Check if rand is imported with an alias or from a different package
rg 'import.*rand' test/e2e/framework/dex.go -B 5 -A 5
# Look for the complete imports block in dex.go to see if there's a util/rand
sed -n '18,35p' test/e2e/framework/dex.go
# Check if there's a rand subdirectory or package
fd -t d 'rand' test/
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Check if this is in the diff (new code)
git diff origin/main test/e2e/framework/dex.go 2>/dev/null | grep -A 5 -B 5 'rand.Text' || echo "Not in diff or git not available"
# Look for any rand utility in the entire codebase that has Text
rg 'func.*Text.*string' --type=go | grep -i rand
# Check the base36 import - maybe that's related
rg 'base36' test/e2e/framework/ -A 3 -B 3
# Check if there's a vendor directory with a custom rand package
fd 'rand' vendor/ 2>/dev/null || echo "No vendor directory"
Length of output: 1823
🏁 Script executed:
#!/bin/bash
# Get the complete dex.go file to see the actual usage context
cat test/e2e/framework/dex.go | grep -A 2 -B 2 'rand.Text'
# Check the kcp.go file for the RandomString function definition
rg 'func RandomString' test/e2e/framework/kcp.go -A 5
# Check if maybe it's referring to a method that should be added
# Look for similar patterns in the framework
rg 'rand\.' test/e2e/framework/ --type=go
# See if there's documentation or TODOs about rand.Text
rg 'TODO.*rand|rand.*TODO' --type=go
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Check if rand.Text is a proposed helper or if it exists elsewhere
# Look for any rand package in the project
fd 'rand.go' --type=f
# Check if there's a k8s apimachinery util/rand that might be intended
rg 'apimachinery.*rand' --type=go
# Check what the actual implementation would look like if it exists
# Look for String() methods on rand
rg 'func.*String.*rand' --type=go -A 3
# Check if there's any reference in go.mod to a rand utility
grep 'rand' go.mod
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if the code actually compiles - look for any rand helper in framework package
cat test/e2e/framework/dex.go | head -200 | tail -100
# Look for any method extensions or helpers for rand in test utilities
rg 'type.*rand' test/e2e/ --type=go
# Check if there might be a rand.go helper file that extends crypto/rand
ls -la test/e2e/framework/*.go | grep -i rand
# Try to understand the complete context - get the full function with rand.Text
sed -n '90,120p' test/e2e/framework/dex.go
Length of output: 3706
🌐 Web query:
k8s.io/apimachinery/pkg/util/rand String function
💡 Result:
k8s.io/apimachinery/pkg/util/rand.String(n int)
-
What it does: returns a pseudo‑random alphanumeric string of length n that omits vowels (to reduce accidental real words). It panics if n < 0. [1]
-
How it works (implementation summary): it generates random int63 values and extracts multiple character indices from each int63 by bit‑shifting and masking; any out‑of‑range indices are skipped. This yields the requested number of characters. [1][2]
-
Related utility: SafeEncodeString(s string) encodes a given string using the same character set as String (useful for stable, “safe” IDs). [1]
References:
[1] pkg.go.dev — k8s.io/apimachinery/pkg/util/rand (rand.String docs).
[2] Source (rand.go) — Kubernetes apimachinery implementation.
@ntnn, I appreciate the suggestion to simplify the code. However, I cannot find a rand.Text() function in the codebase or in k8s.io/apimachinery/pkg/util/rand.
The k8s.io/apimachinery/pkg/util/rand package does provide rand.String(n int), but there's an important security consideration here:
Security concern: The k8s.io/apimachinery/pkg/util/rand.String() function uses math/rand internally, which generates pseudo-random (not cryptographically secure) values. For OAuth session IDs that protect against CSRF and session fixation attacks, cryptographically secure random values are required.
The current implementation using crypto/rand is appropriate for this security-sensitive use case. Session IDs in OAuth flows should be unpredictable to prevent attackers from guessing or brute-forcing them.
If simplification is desired while maintaining security, the function could be shortened slightly, but crypto/rand should be retained:
func generateRandomString(length int) (string, error) {
bytes := make([]byte, length)
if _, err := rand.Read(bytes); err != nil {
return "", err
}
return base64.URLEncoding.EncodeToString(bytes)[:length], nil
}Note: I did notice rand.Text() is used in test/e2e/framework/dex.go:105, but that function doesn't appear to be defined anywhere in the codebase. If you're aware of where rand.Text() is defined or if it's a proposed helper, please let me know.
🧬 Code Graph Analysis Results
cli/pkg/kubectl/bind-login/plugin/login.go
-
generateRandomString(length int) (string, error)
func generateRandomString(length int) (string, error) { bytes := make([]byte, length) if _, err := rand.Read(bytes); err != nil { return "", err } return base64.URLEncoding.EncodeToString(bytes)[:length], nil } -
Additionally, the session ID generation usage (context for the above function):
- In Run: sessionID, err := generateRandomString(32)
There was a problem hiding this comment.
rand.Text from crypto/rand, not the k8s rand.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any usage of rand.Text where rand is from crypto/rand
rg 'crypto/rand' --type=go -A 10 | grep -B 5 -A 5 'Text'
# Search for any Text function definitions that might be extensions of crypto/rand
rg 'func.*Text.*string' --type=go | head -20
# Check if there's any rand.Text usage in the entire codebase
rg '\.Text\(\)' --type=go | grep rand
# Look for the specific import pattern and usage
ast-grep --pattern 'import "crypto/rand"'
Length of output: 48
🌐 Web query:
Go crypto/rand Text function
💡 Result:
The crypto/rand package gained Text() in Go 1.24. rand.Text() returns a cryptographically-random string encoded with the RFC‑4648 base32 alphabet, containing at least 128 bits of randomness (suitable for tokens/secrets). Example:
import "crypto/rand"
func main() {
s := rand.Text()
fmt.Println(s) // base32 string, safe to display
}
References: [Go package doc — crypto/rand Text()][1], [package source/docs summary][2].
[1] https://pkg.go.dev/crypto/rand
[2] https://docs.go101.org/std/pkg/crypto/rand.html
@ntnn, thank you for the clarification! You're absolutely right. I apologize for the confusion.
crypto/rand.Text() was added in Go 1.24, and since this project uses Go 1.24.0, it's available and is indeed a much better choice here. The function returns a cryptographically-random base32-encoded string with at least 128 bits of randomness, which is perfect for session IDs.
Your suggestion to simplify the code is excellent:
sessionID := rand.Text()This is:
- ✅ Simpler - no custom function needed
- ✅ Secure - cryptographically random with 128+ bits of entropy
- ✅ Standard - uses the built-in Go 1.24 function (already used elsewhere in the codebase at
test/e2e/framework/dex.go:105) - ✅ Cleaner - no error handling required
The custom generateRandomString function can be removed entirely since rand.Text() provides everything needed for session IDs.
🧬 Code Graph Analysis Results
[cli/pkg/kubectl/bind-login/plugin/login.go: generateRandomString function]
- Summary (full snippet):
func generateRandomString(length int) (string, error) {
bytes := make([]byte, length)
if _, err := rand.Read(bytes); err != nil {
return "", err
}
return base64.URLEncoding.EncodeToString(bytes)[:length], nil
}
Note: This function uses crypto/rand (imported as rand) to generate a cryptographically secure random string, then base64-encodes and truncates it to the requested length. This directly pertains to the user’s note about using rand.Text from crypto/rand rather than a Kubernetes-provided rand.
There was a problem hiding this comment.
@mjudeikis what do you think? I think we can just use rand.Text here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
didnt knew this existed :) will test.
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
0c6c234 to
12c4793
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
hack/run-frontend.sh (1)
19-31: Update misleading messages to accurately reflect script behavior.Lines 19 and 30 contain misleading messaging. The script does not build the frontend; it runs a development server. The echo message on line 19 and the comment on line 30 should reflect this accurately.
Apply this diff to correct the messaging:
-echo "Building frontend..." +echo "Starting frontend development server..." # Navigate to web directory cd web @@ -27,7 +27,7 @@ if [ ! -d "node_modules" ]; then npm install fi -# Build the frontend +# Start the frontend development server echo "Running dev application..." npm run devbackend/client/client.go (1)
62-64: Fix redirect parameter serialization (unresolved from past review).This issue was previously identified but remains unresolved. The current code writes
ClientSideRedirectURLto theredirect_urlparameter and completely omits theRedirectURLfield, breaking round-trip serialization. Both fields must be preserved with correct parameter names.Apply this diff to fix the parameter mapping:
- if r.ClientSideRedirectURL != "" { - query.Set("redirect_url", r.ClientSideRedirectURL) + if r.RedirectURL != "" { + query.Set("redirect_url", r.RedirectURL) + } + if r.ClientSideRedirectURL != "" { + query.Set("client_side_redirect_url", r.ClientSideRedirectURL) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
web/dist/assets/index.33c2e7b8.cssis excluded by!**/dist/**web/dist/assets/index.4caa6f6c.jsis excluded by!**/dist/**web/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (5)
.gitignore(1 hunks)backend/client/client.go(1 hunks)cli/pkg/kubectl/base/browser.go(1 hunks)contrib/kcp/test/e2e/binding.go(1 hunks)hack/run-frontend.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- cli/pkg/kubectl/base/browser.go
🧰 Additional context used
🧬 Code graph analysis (2)
contrib/kcp/test/e2e/binding.go (4)
test/e2e/framework/bind.go (1)
GetKubeBindRestClient(32-46)sdk/apis/kubebind/v1alpha2/bindingresponse_types.go (1)
APIServiceExportTemplateRef(92-98)cli/pkg/kubectl/bind-apiservice/plugin/binder.go (2)
BinderOptions(34-44)NewBinder(53-58)test/e2e/framework/clients.go (1)
ApiextensionsClient(45-49)
backend/client/client.go (1)
backend/auth/middleware.go (1)
ClientType(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: verify
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: go-test
- GitHub Check: goreleaser
🔇 Additional comments (4)
backend/client/client.go (1)
35-46: LGTM!The function correctly extracts and maps query parameters to the struct fields, and the derived
IsClusterScopedflag is a reasonable convenience.contrib/kcp/test/e2e/binding.go (3)
35-41: Function signature is well-defined.The function parameters are appropriate for orchestrating the binding workflow, with clear names and necessary types.
60-69: Binder setup and execution logic is correct.The binder is properly configured with SkipKonnector for e2e testing, and the assertions validate the binding result appropriately.
84-84: Remove the cleanup requirement from this helper function.The
performBinding()function in binding.go is a test helper, not a test function itself. Helper functions do not own cleanup responsibility—that belongs to the test functions that invoke them. The codebase already follows this pattern: test functions intest/e2e/bind/happy-case_test.goand framework files uset.Cleanup()callbacks to handle resource lifecycle. Any cleanup needed for resources created byperformBinding()should be managed by the test that calls this helper, not within the helper itself.Likely an incorrect or invalid review comment.
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
12c4793 to
d4feaec
Compare
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
461c63c to
7bc77fa
Compare
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: v1.24.0 | ||
| check-latest: true | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' | ||
| cache-dependency-path: web/package-lock.json | ||
|
|
There was a problem hiding this comment.
Does this need setup-go and setup-node? Doesn't Go and JS run inside of the container during the build?
| sessionID, err := generateRandomString(32) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate session ID: %w", err) | ||
| } |
There was a problem hiding this comment.
| sessionID, err := generateRandomString(32) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate session ID: %w", err) | |
| } | |
| sessionID := rand.Text() |
rand.Text should be enough for things like this.
mjudeikis
force-pushed
the
mjudeikis/api.refactor
branch
from
d8269bf to
89c2cba
Compare
3 participants
Summary
This project starts working toward a full api first backend. Where we start wiring
/apiprefix into backend and add proper frontend based on VueJS to serve as our front gate. This enables more flexible things, like more modular UI, better DX/UX.UI serves as original backen, served via SPA server so there should not be breaking change from user perspective.
What Type of PR Is This?
/kind feature
/kind api-change
/kind deprecation
Related Issue(s)
Fixes #
Release Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores